Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthrough📝 WalkthroughUpdates across UI, controllers, services, migrations, and tests: adds unit-aware speed display, tightens geodata/coordinate validation, refactors map upgrade banner logic to a 12-month gate, adds deadlock-retry for reverse-geocoding bulk writes, improves user admin CRUD and registration toggles, introduces several schema/index migrations, and expands test coverage and rake tooling. Changes
Sequence Diagram(s)(omitted — changes are primarily bug fixes, refactors, UI tweaks and test additions; no sequence diagram generated) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/javascript/controllers/maps/maplibre/map_data_manager.js (1)
374-385:⚠️ Potential issue | 🟠 MajorDismiss stale Lite banners when the range becomes valid again.
This helper only ever calls
UpgradeBanner.show(). If a user first loads an out-of-window range and then narrows the filter back inside the last 12 months, the old banner stays visible until they reload or dismiss it manually.Suggested fix
_showDataWindowBanner() { const startDate = new Date(this.controller.startDateValue) const twelveMonthsAgo = new Date() twelveMonthsAgo.setMonth(twelveMonthsAgo.getMonth() - 12) - if (startDate < twelveMonthsAgo) { + if (Number.isNaN(startDate.getTime())) { + UpgradeBanner.dismiss() + return + } + + if (startDate < twelveMonthsAgo) { UpgradeBanner.show({ message: "Your Lite plan includes the last 12 months of data.", upgradeUrl: this.controller.upgradeUrlValue, utmContent: "data_retention", }) + } else { + UpgradeBanner.dismiss() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/controllers/maps/maplibre/map_data_manager.js` around lines 374 - 385, The _showDataWindowBanner helper only shows the UpgradeBanner and never clears it; update _showDataWindowBanner (in map_data_manager.js) to hide/dismiss the banner when the date range becomes valid again by adding an else branch that calls the banner dismissal method (e.g., UpgradeBanner.hide() or UpgradeBanner.dismiss() depending on the existing API) instead of doing nothing; keep the existing show() call for the out-of-window case and ensure you reference the same UpgradeBanner symbol so stale banners are removed when startDate >= twelveMonthsAgo.
🧹 Nitpick comments (2)
spec/services/immich/import_geodata_spec.rb (1)
95-149: Add the symmetriclongitude == 0case.These new examples cover the
latitude == 0path, but the new validation also rejectslongitude == 0, and that branch is still untested. Alatitude: 52.11, longitude: 0example would close the gap without coupling the spec to internals. As per coding guidelines, "Test behavior, not implementation."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/services/immich/import_geodata_spec.rb` around lines 95 - 149, Add a symmetric spec for the longitude==0 branch by creating a new context (e.g., "when photo has zero longitude only") that defines immich_data with exifInfo latitude: 52.11 and longitude: 0 (keeping other fields the same as the other examples) and assert the same behavior: expect { service }.not_to(change { Import.count }); this mirrors the existing "zero latitude only" test and ensures the longitude==0 validation path is covered.spec/requests/visits_spec.rb (1)
124-129: Consider adding buttons replacement assertion to the failure test.The controller renders both
visit_nameandvisit_buttonsreplacements on failure (perapp/controllers/visits_controller.rblines 41-44), but this test only assertsvisit_name. For consistency with the success path test, consider adding the buttons assertion.Suggested improvement
it 'returns turbo_stream replace even on failure' do patch visit_url(visit), params: { visit: { status: :confirmed } }, as: :turbo_stream expect_turbo_stream_response expect_turbo_stream_action('replace', "visit_name_#{visit.id}") + expect_turbo_stream_action('replace', "visit_buttons_#{visit.id}") end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/requests/visits_spec.rb` around lines 124 - 129, The test "returns turbo_stream replace even on failure" only asserts the replacement of "visit_name_#{visit.id}" but the controller also renders a replacement for "visit_buttons_#{visit.id}"; update the spec to also assert expect_turbo_stream_action('replace', "visit_buttons_#{visit.id}") (using the same expect_turbo_stream_response/expect_turbo_stream_action helper) so the failure-path test matches the success-path expectations and covers both rendered turbo_stream targets.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/assets/stylesheets/maps_maplibre.css`:
- Around line 237-240: The upgrade banner (.map-upgrade-banner) currently shares
the same top/left/transform and z-index as the progress badge
(.map-progress-badge), causing them to overlap; update the .map-upgrade-banner
rules to avoid that slot by giving it a distinct vertical offset or higher
stacking context — for example change its top to a different value (e.g., move
it up or down relative to 16px) or set its z-index above the .map-progress-badge
(use a higher z-index than the badge) so the banner no longer occupies the exact
same top-center position as .map-progress-badge.
In `@app/javascript/controllers/maps_controller.js`:
- Around line 338-357: The Lite upgrade banner created by UpgradeBanner.show()
is never removed; update showDataWindowBanner() to (1) use Stimulus static
values for user_plan, start_date and upgrade_url (e.g., userPlanValue,
startDateValue, upgradeUrlValue) instead of reading this.element.dataset
directly, (2) store the result/DOM reference returned by UpgradeBanner.show() on
the controller instance (e.g., this.upgradeBannerEl) so you can track it, (3) if
the banner no longer applies (user not gated or startDate within 12 months)
ensure you remove/close the stored banner (call the appropriate teardown API or
remove the element) and clear this.upgradeBannerEl, and (4) implement
disconnect() to always clean up any remaining this.upgradeBannerEl (remove
DOM/listeners) so the banner cannot persist across navigations. Ensure you
reference showDataWindowBanner(), UpgradeBanner.show(), and disconnect() when
making the changes.
In `@app/javascript/controllers/maps/maplibre/map_data_manager.js`:
- Around line 83-85: In loadMapData(), the Lite upsell banner
(_showDataWindowBanner) is invoked while the progress/loading badge is still
visible, causing an overlap; modify loadMapData() to call _showDataWindowBanner
only after the progress badge has been dismissed (i.e., move the
isGatedPlan(this.controller.userPlanValue) check and call to
_showDataWindowBanner to run after the code path that dismisses/hides the
loading/progress badge completes), ensuring the banner is shown after the badge
hide/dismiss operation finishes.
In `@spec/serializers/api/digest_list_serializer_spec.rb`:
- Around line 11-49: The spec expects Api::DigestListSerializer to serialize
each digest's distance as a hash with keys :converted, :unit and :meters and to
accept a distance_unit argument (defaulting to 'km') and available_years; update
the serializer (class/method names: Api::DigestListSerializer, .new(...).call
and any private build_digest/build_distance helpers) so that instead of
serializing distance: digest.distance it builds a distance object: compute
meters from digest.distance, convert to km or mi depending on the distance_unit
argument, set :converted and :unit accordingly, and still include :meters;
ensure the serializer initializer accepts distance_unit (default 'km') and
available_years and that call returns availableYears in the top-level hash to
satisfy the spec.
---
Outside diff comments:
In `@app/javascript/controllers/maps/maplibre/map_data_manager.js`:
- Around line 374-385: The _showDataWindowBanner helper only shows the
UpgradeBanner and never clears it; update _showDataWindowBanner (in
map_data_manager.js) to hide/dismiss the banner when the date range becomes
valid again by adding an else branch that calls the banner dismissal method
(e.g., UpgradeBanner.hide() or UpgradeBanner.dismiss() depending on the existing
API) instead of doing nothing; keep the existing show() call for the
out-of-window case and ensure you reference the same UpgradeBanner symbol so
stale banners are removed when startDate >= twelveMonthsAgo.
---
Nitpick comments:
In `@spec/requests/visits_spec.rb`:
- Around line 124-129: The test "returns turbo_stream replace even on failure"
only asserts the replacement of "visit_name_#{visit.id}" but the controller also
renders a replacement for "visit_buttons_#{visit.id}"; update the spec to also
assert expect_turbo_stream_action('replace', "visit_buttons_#{visit.id}") (using
the same expect_turbo_stream_response/expect_turbo_stream_action helper) so the
failure-path test matches the success-path expectations and covers both rendered
turbo_stream targets.
In `@spec/services/immich/import_geodata_spec.rb`:
- Around line 95-149: Add a symmetric spec for the longitude==0 branch by
creating a new context (e.g., "when photo has zero longitude only") that defines
immich_data with exifInfo latitude: 52.11 and longitude: 0 (keeping other fields
the same as the other examples) and assert the same behavior: expect { service
}.not_to(change { Import.count }); this mirrors the existing "zero latitude
only" test and ensures the longitude==0 validation path is covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f30c7c4c-284c-481a-8976-9be82f76acfb
📒 Files selected for processing (19)
CHANGELOG.mdapp/assets/stylesheets/maps_maplibre.cssapp/controllers/visits_controller.rbapp/helpers/application_helper.rbapp/javascript/controllers/maps/maplibre/data_loader.jsapp/javascript/controllers/maps/maplibre/map_data_manager.jsapp/javascript/controllers/maps_controller.jsapp/services/immich/import_geodata.rbapp/views/insights/index.html.erbapp/views/map/leaflet/index.html.erbapp/views/map/maplibre/index.html.erbapp/views/points/_point.html.erbapp/views/points/index.html.erbapp/views/shared/map/_upgrade_banner.html.erbapp/views/visits/_buttons.html.erbspec/helpers/application_helper_spec.rbspec/requests/visits_spec.rbspec/serializers/api/digest_list_serializer_spec.rbspec/services/immich/import_geodata_spec.rb
💤 Files with no reviewable changes (1)
- app/views/map/maplibre/index.html.erb
Comment on lines
+237
to
+240
| top: 16px; | ||
| left: 50%; | ||
| transform: translateX(-50%); | ||
| z-index: 1000; | ||
| z-index: 20; |
Contributor
There was a problem hiding this comment.
Keep the upgrade banner out of the progress badge slot.
.map-upgrade-banner now shares the same top-center position and z-index as .map-progress-badge, so gated loads can briefly stack both overlays in the same spot. Give the banner its own offset or let it sit above the badge.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/assets/stylesheets/maps_maplibre.css` around lines 237 - 240, The upgrade
banner (.map-upgrade-banner) currently shares the same top/left/transform and
z-index as the progress badge (.map-progress-badge), causing them to overlap;
update the .map-upgrade-banner rules to avoid that slot by giving it a distinct
vertical offset or higher stacking context — for example change its top to a
different value (e.g., move it up or down relative to 16px) or set its z-index
above the .map-progress-badge (use a higher z-index than the badge) so the
banner no longer occupies the exact same top-center position as
.map-progress-badge.
Comment on lines
+338
to
357
| // Show upgrade banner for Lite users when searching outside the 12-month window | ||
| this.showDataWindowBanner() | ||
| } | ||
|
|
||
| showDataWindowBanner() { | ||
| const userPlan = this.element.dataset.user_plan | ||
| if (!isGatedPlan(userPlan)) return | ||
|
|
||
| const startDate = new Date(this.element.dataset.start_date) | ||
| const twelveMonthsAgo = new Date() | ||
| twelveMonthsAgo.setMonth(twelveMonthsAgo.getMonth() - 12) | ||
|
|
||
| if (startDate < twelveMonthsAgo) { | ||
| UpgradeBanner.show({ | ||
| message: "Your Lite plan includes the last 12 months of data.", | ||
| upgradeUrl: this.element.dataset.upgrade_url, | ||
| utmContent: "data_retention", | ||
| }) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Clean up the injected upgrade banner when it no longer applies.
UpgradeBanner.show() appends DOM outside the Leaflet lifecycle, but this controller never dismisses it when the current range is allowed or when the controller disconnects. A Lite banner shown on one map visit can therefore survive navigation to another range or off the map page entirely. As per coding guidelines: "Stimulus controllers should use static targets for DOM references, static values for data from HTML attributes. Always clean up in disconnect() (event listeners, timers, subscriptions)."
Suggested fix
showDataWindowBanner() {
const userPlan = this.element.dataset.user_plan
- if (!isGatedPlan(userPlan)) return
+ if (!isGatedPlan(userPlan)) {
+ UpgradeBanner.dismiss()
+ return
+ }
const startDate = new Date(this.element.dataset.start_date)
const twelveMonthsAgo = new Date()
twelveMonthsAgo.setMonth(twelveMonthsAgo.getMonth() - 12)
if (startDate < twelveMonthsAgo) {
UpgradeBanner.show({
message: "Your Lite plan includes the last 12 months of data.",
upgradeUrl: this.element.dataset.upgrade_url,
utmContent: "data_retention",
})
+ } else {
+ UpgradeBanner.dismiss()
}
}
+
+ disconnect() {
+ UpgradeBanner.dismiss()
+ super.disconnect()
+ this.removeEventListeners()
+ ...
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/javascript/controllers/maps_controller.js` around lines 338 - 357, The
Lite upgrade banner created by UpgradeBanner.show() is never removed; update
showDataWindowBanner() to (1) use Stimulus static values for user_plan,
start_date and upgrade_url (e.g., userPlanValue, startDateValue,
upgradeUrlValue) instead of reading this.element.dataset directly, (2) store the
result/DOM reference returned by UpgradeBanner.show() on the controller instance
(e.g., this.upgradeBannerEl) so you can track it, (3) if the banner no longer
applies (user not gated or startDate within 12 months) ensure you remove/close
the stored banner (call the appropriate teardown API or remove the element) and
clear this.upgradeBannerEl, and (4) implement disconnect() to always clean up
any remaining this.upgradeBannerEl (remove DOM/listeners) so the banner cannot
persist across navigations. Ensure you reference showDataWindowBanner(),
UpgradeBanner.show(), and disconnect() when making the changes.
Comment on lines
+83
to
+85
| // 5. Show upsell banner for Lite users when searching outside the 12-month window | ||
| if (isGatedPlan(this.controller.userPlanValue)) { | ||
| this._showDataWindowBanner(data.totalPointsInRange, data.points.length) | ||
| this._showDataWindowBanner() |
Contributor
There was a problem hiding this comment.
Show the upgrade banner after the loading badge is gone.
loadMapData() renders the Lite upsell before the progress badge is dismissed, and both overlays occupy the same top-center slot in app/assets/stylesheets/maps_maplibre.css. That produces a visible overlap at the end of gated loads.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/javascript/controllers/maps/maplibre/map_data_manager.js` around lines 83
- 85, In loadMapData(), the Lite upsell banner (_showDataWindowBanner) is
invoked while the progress/loading badge is still visible, causing an overlap;
modify loadMapData() to call _showDataWindowBanner only after the progress badge
has been dismissed (i.e., move the isGatedPlan(this.controller.userPlanValue)
check and call to _showDataWindowBanner to run after the code path that
dismisses/hides the loading/progress badge completes), ensuring the banner is
shown after the badge hide/dismiss operation finishes.
Update user management
Introduce number of db optimizations
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (4)
spec/services/reverse_geocoding/points/fetch_data_spec.rb (1)
128-155: Good test coverage for the new guard clauses.The use of
update_columnto bypass validations is appropriate for simulating legacy/invalid data scenarios.Minor cleanup: the
let(:point)declarations on lines 129 and 144 are identical to the default on line 8 and can be removed.♻️ Optional: Remove redundant let declarations
context 'when point has nil timestamp' do - let(:point) { create(:point) } - before do # Bypass validations to simulate legacy data with nil timestamp point.update_column(:timestamp, nil) endcontext 'when point has nil lonlat' do - let(:point) { create(:point) } - before do point.update_column(:lonlat, nil) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/services/reverse_geocoding/points/fetch_data_spec.rb` around lines 128 - 155, Remove the redundant local let(:point) declarations inside the two contexts that duplicate the top-level let(:point) defined earlier in the spec; specifically delete the inner let(:point) in the "when point has nil timestamp" and "when point has nil lonlat" contexts so they inherit the top-level factory, leaving the setup blocks (point.update_column ...) and examples intact.spec/services/users/import_data_spec.rb (1)
108-129: Prefer behavior-driven setup over stubbing internal detection.This context currently stubs
detect_format_version, which couples the spec to internals. A stronger spec would build an archive lacking bothmanifest.jsonanddata.jsonand assert the same observable outcomes.As per coding guidelines: “spec/**/*.rb: Test behavior, not implementation… Avoid over-stubbing.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/services/users/import_data_spec.rb` around lines 108 - 129, The test over-stubs internal behavior by mocking Users::ImportData#detect_format_version and should instead exercise the public path: build a real archive payload missing both "manifest.json" and "data.json", call service.import, and assert it returns nil and creates the failure notification without calling ExceptionReporter; remove the allow(service).to receive(:detect_format_version) and allow(service).to receive(:extract_archive) stubs and replace them by preparing the archive fixture/IO that triggers Users::ImportData::UnsupportedFormatError during import so the spec verifies observable behavior from service.import and uses notification expectations around ::Notifications::Create and ExceptionReporter as currently written.app/services/reverse_geocoding/places/fetch_data.rb (1)
161-170: De-correlate the retry backoff.Every worker currently sleeps for the same 100/200/300 ms intervals. In the concurrent Sidekiq workload this change is targeting, that can wake contending jobs back up together and re-deadlock on the same rows.
Proposed tweak
- sleep(0.1 * retries) + sleep((0.1 * retries) + rand(0.0..0.05))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/reverse_geocoding/places/fetch_data.rb` around lines 161 - 170, The with_deadlock_retry method uses a deterministic backoff (sleep(0.1 * retries)) which causes concurrent jobs to wake simultaneously and re-deadlock; change it to use a de‑correlated backoff by adding jitter (e.g., randomize each sleep interval or use exponential backoff multiplied by a random factor) before retrying, keeping the existing retry cap (DEADLOCK_MAX_RETRIES) and still raising the ActiveRecord::Deadlocked error once retries are exhausted.spec/services/reverse_geocoding/places/fetch_data_spec.rb (1)
446-465: Drive this through#call, notsend(:save_places).These examples are asserting a private helper and stubbing
service.sleep, so they will break on internal refactors even ifReverseGeocoding::Places::FetchData#callstill behaves correctly. Cover the deadlock via the public entrypoint and assert on the persisted result / raised error instead.As per coding guidelines, "Test behavior, not implementation. Never mock the object under test. Never test private methods via
send()."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/services/reverse_geocoding/places/fetch_data_spec.rb` around lines 446 - 465, Change the examples to exercise the public entrypoint ReverseGeocoding::Places::FetchData#call instead of calling the private helper save_places via send; e.g., trigger service.call and drive the deadlock scenarios by stubbing Place.insert_all to raise/then succeed (or always raise) and, if needed, stub service.sleep to avoid real delays, then assert on the persisted result (Place.count change) for the retry-success case and assert that service.call raises ActiveRecord::Deadlocked after exhausted retries for the failure case — remove direct references to save_places and any tests that call private methods via send.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/controllers/settings/users_controller.rb`:
- Around line 23-27: Wrap the invariant check and the update inside a database
transaction with row-level locks so concurrent requests cannot both pass the
last-admin check; specifically, in UsersController where you currently call
last_admin_protection_needed? before `@user.update`(filtered_user_params), perform
an ActiveRecord::Base.transaction that obtains a FOR UPDATE lock on the relevant
admin rows (e.g. User.where(role: 'admin', enabled: true).lock) and also lock
the `@user` record (e.g. `@user.lock`! or `@user.reload`(lock: true)), re-evaluate
last_admin_protection_needed? inside that transaction, and only then call
`@user.update`(update_params); apply the same transactional lock pattern to the
other code path referenced (the action around lines 146-147) to ensure atomic
demote/disable operations.
- Around line 142-147: The controller currently treats any non-"active" status
as a disable and counts all admins (including inactive/trial) as part of the
sole-admin guard; update disabling_user? to only detect an actual transition
into a disabled state by checking that user_params[:status] is present and the
new status is not an enabled status (e.g. not "active" or "trial" per
app/models/user.rb's active_or_trial semantics), and update sole_admin? to only
count enabled admins (use the active_or_trial scope or method when filtering
admins, e.g. User.active_or_trial.where(admin: true).count == 1) so
inactive/trial users are not treated as enabled admins.
In `@app/helpers/application_helper.rb`:
- Around line 56-61: point_speed currently returns the raw value for
non-positive speeds, causing mismatch with speed_label which always shows "km/h"
or "mph"; update point_speed (and/or speed_label usage) so negative/zero
readings are converted to the same units as positives: remove the early "return
speed if speed.to_f <= 0" behavior and always convert using kmh = speed.to_f *
3.6 and the mi branch (unit == 'mi' ? (kmh * 0.621371).round(1) : kmh.round(1));
alternatively, if you must preserve raw negatives, make speed_label check for
raw-meter-per-second values and avoid appending "km/h"/"mph" for those cases
(reference point_speed and speed_label to keep behavior consistent).
In `@app/services/users/import_data.rb`:
- Line 170: The code currently raises UnsupportedFormatError for missing markers
but still raises a generic StandardError in create_handler when encountering an
unknown manifest format_version; update create_handler (the method that parses
manifest and selects handlers) to raise UnsupportedFormatError instead of
StandardError for unsupported or unknown format_version values so error handling
is consistent—replace the generic exception raising logic in create_handler to
raise UnsupportedFormatError with a clear message that includes the offending
format_version and context.
- Around line 71-73: The rescue for UnsupportedFormatError in import_data.rb
currently calls create_failure_notification(e) and returns nil which swallows
the failure; instead either re-raise the exception or return an explicit failure
result so callers (e.g., Users::ImportDataJob) do not proceed on a false
success. Modify the rescue in the method that rescues UnsupportedFormatError to
call create_failure_notification(e) and then raise e (or return a structured
failure object like Result.failure or a boolean false) so callers can detect and
handle the import failure rather than receiving nil.
In `@app/views/settings/users/index.html.erb`:
- Around line 47-48: The actions column on the users index is missing a visible
entry point to the user's details page (where API-key rotation and
password-reset live); update the actions cell in
app/views/settings/users/index.html.erb to include a visible link to
settings_user_path(user) (e.g., "Details" or an eye icon) alongside the existing
delete action so admins can reach the show flows, and apply the same addition to
the other actions cell instances referenced around the second block (the 87-91
area) to ensure all rows include the details link.
- Around line 12-20: The registration checkbox lacks an accessible label and a
non-JS fallback submit; update the form generated by form_with
(update_registration_settings_settings_users_path) so the checkbox input named
registration_enabled has a unique id (e.g., registration_enabled) and an
associated <label> element that references that id (or use aria-labelledby) with
visible text "User registration", keep the onchange submit for JS but also add a
standard submit control (a visible or visually-hidden <button type="submit"> or
a <noscript> fallback) so assistive tech and non-JS users can change and submit
the setting; ensure the label text matches the copy used in the surrounding
header for clarity.
In `@app/views/settings/users/show.html.erb`:
- Around line 107-119: The view currently prints `@user.api_key` directly into the
DOM (see the code block rendering `@user.api_key` and the
regenerate_api_key_settings_user_path button); change this to avoid showing a
live API key by default: render a masked placeholder (e.g., "••••••••••") or
only show the last 4 characters, and add UI that reveals the full key only
immediately after a successful regeneration (or via a one-time secure response
from the controller that returns the new key to the client). Update the show
template to stop embedding `@user.api_key`, adjust the controller/action that
handles regenerate_api_key_settings_user_path to return the new key in the
response for one-time display, and ensure any client-side logic clears the
displayed key after navigation or timeout.
In `@CLAUDE.md`:
- Line 40: Update the phrase in the "Turbo first" guideline to use a hyphenated
compound adjective: replace "full page reloads" with "full-page reloads" within
the sentence that currently reads "use Turbo Frames and Turbo Streams/broadcasts
wherever appropriate to avoid full page reloads and provide smooth, in-place UI
updates." Ensure the edited sentence preserves the surrounding punctuation and
capitalization.
In `@config/initializers/03_dawarich_settings.rb`:
- Around line 65-70: Normalize the registration flag stored in cache by
enforcing boolean casting in both registration_enabled? and
set_registration_enabled: ensure set_registration_enabled(enabled) converts
incoming values (e.g., strings like "0"/"false", integers, nil) into a
true/false boolean before writing to Rails.cache under the
'dawarich/registration_enabled' key, and ensure registration_enabled? reads the
cached value and returns a normalized boolean fallback to
ALLOW_EMAIL_PASSWORD_REGISTRATION when cache is empty; update the methods
(registration_enabled? and set_registration_enabled) to perform this
normalization so the cached value cannot be a truthy string.
In `@db/migrate/20260125100000_enqueue_transportation_mode_backfill_jobs.rb`:
- Around line 51-53: The raw SQL builds a source IN (...) using string enum
names (supported_sources) but imports.source is an integer-backed enum, so
replace the string names with their integer values before building the SQL; map
each element of supported_sources to its corresponding integer (via the model
enum mapping or Import.sources[s]) and use those integers in the execute call
that populates import_ids (the execute("SELECT id FROM imports WHERE source IN
(...)") line) so the query returns rows instead of being silently masked by the
rescue.
In `@spec/requests/settings/users_spec.rb`:
- Around line 299-317: The tests under describe 'PATCH
/update_registration_settings' mutate global cache via
DawarichSettings.set_registration_enabled and leave state for other specs; wrap
these examples to save and restore the original flag (use
DawarichSettings.registration_enabled? to read current value), for example with
an around block or ensure/finally inside each example so you call
DawarichSettings.set_registration_enabled(previous_value) after the test; update
the examples that call DawarichSettings.set_registration_enabled and the ones
asserting DawarichSettings.registration_enabled? to restore the prior value to
avoid leaking state.
---
Nitpick comments:
In `@app/services/reverse_geocoding/places/fetch_data.rb`:
- Around line 161-170: The with_deadlock_retry method uses a deterministic
backoff (sleep(0.1 * retries)) which causes concurrent jobs to wake
simultaneously and re-deadlock; change it to use a de‑correlated backoff by
adding jitter (e.g., randomize each sleep interval or use exponential backoff
multiplied by a random factor) before retrying, keeping the existing retry cap
(DEADLOCK_MAX_RETRIES) and still raising the ActiveRecord::Deadlocked error once
retries are exhausted.
In `@spec/services/reverse_geocoding/places/fetch_data_spec.rb`:
- Around line 446-465: Change the examples to exercise the public entrypoint
ReverseGeocoding::Places::FetchData#call instead of calling the private helper
save_places via send; e.g., trigger service.call and drive the deadlock
scenarios by stubbing Place.insert_all to raise/then succeed (or always raise)
and, if needed, stub service.sleep to avoid real delays, then assert on the
persisted result (Place.count change) for the retry-success case and assert that
service.call raises ActiveRecord::Deadlocked after exhausted retries for the
failure case — remove direct references to save_places and any tests that call
private methods via send.
In `@spec/services/reverse_geocoding/points/fetch_data_spec.rb`:
- Around line 128-155: Remove the redundant local let(:point) declarations
inside the two contexts that duplicate the top-level let(:point) defined earlier
in the spec; specifically delete the inner let(:point) in the "when point has
nil timestamp" and "when point has nil lonlat" contexts so they inherit the
top-level factory, leaving the setup blocks (point.update_column ...) and
examples intact.
In `@spec/services/users/import_data_spec.rb`:
- Around line 108-129: The test over-stubs internal behavior by mocking
Users::ImportData#detect_format_version and should instead exercise the public
path: build a real archive payload missing both "manifest.json" and "data.json",
call service.import, and assert it returns nil and creates the failure
notification without calling ExceptionReporter; remove the allow(service).to
receive(:detect_format_version) and allow(service).to receive(:extract_archive)
stubs and replace them by preparing the archive fixture/IO that triggers
Users::ImportData::UnsupportedFormatError during import so the spec verifies
observable behavior from service.import and uses notification expectations
around ::Notifications::Create and ExceptionReporter as currently written.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cee79b2f-640f-4d04-afe5-df1aaebe5285
📒 Files selected for processing (28)
.app_versionCHANGELOG.mdCLAUDE.mdapp/assets/builds/tailwind.cssapp/controllers/settings/users_controller.rbapp/controllers/users/registrations_controller.rbapp/controllers/users/sessions_controller.rbapp/helpers/application_helper.rbapp/services/reverse_geocoding/places/fetch_data.rbapp/services/reverse_geocoding/points/fetch_data.rbapp/services/users/destroy.rbapp/services/users/import_data.rbapp/views/settings/users/edit.html.erbapp/views/settings/users/index.html.erbapp/views/settings/users/show.html.erbconfig/initializers/03_dawarich_settings.rbconfig/initializers/filter_parameter_logging.rbconfig/routes.rbdb/migrate/20260112192240_set_existing_users_to_map_v1.rbdb/migrate/20260125100000_enqueue_transportation_mode_backfill_jobs.rbdb/migrate/20260206202634_deduplicate_tracks.rbdb/schema.rbdocker/docker-compose.ymlspec/requests/settings/users_spec.rbspec/services/reverse_geocoding/places/fetch_data_spec.rbspec/services/reverse_geocoding/points/fetch_data_spec.rbspec/services/users/destroy_spec.rbspec/services/users/import_data_spec.rb
✅ Files skipped from review due to trivial changes (1)
- .app_version
Comment on lines
+23
to
+27
| return redirect_to settings_users_url, alert: last_admin_alert_message if last_admin_protection_needed? | ||
|
|
||
| update_params = filtered_user_params | ||
|
|
||
| if @user.update(update_params) |
Contributor
There was a problem hiding this comment.
Make the last-admin protection atomic.
Line 23 checks the invariant before Line 27 writes the row, so two concurrent demote/disable requests can both pass and leave the system with no enabled admin. This needs to run inside a single transaction with locks, or be enforced at the database level.
Also applies to: 146-147
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/settings/users_controller.rb` around lines 23 - 27, Wrap the
invariant check and the update inside a database transaction with row-level
locks so concurrent requests cannot both pass the last-admin check;
specifically, in UsersController where you currently call
last_admin_protection_needed? before `@user.update`(filtered_user_params), perform
an ActiveRecord::Base.transaction that obtains a FOR UPDATE lock on the relevant
admin rows (e.g. User.where(role: 'admin', enabled: true).lock) and also lock
the `@user` record (e.g. `@user.lock`! or `@user.reload`(lock: true)), re-evaluate
last_admin_protection_needed? inside that transaction, and only then call
`@user.update`(update_params); apply the same transactional lock pattern to the
other code path referenced (the action around lines 146-147) to ensure atomic
demote/disable operations.
Comment on lines
+142
to
+147
| def disabling_user? | ||
| user_params.key?(:status) && user_params[:status] != 'active' | ||
| end | ||
|
|
||
| def sole_admin? | ||
| User.where(admin: true).count == 1 |
Contributor
There was a problem hiding this comment.
Don't count inactive admins as satisfying the last-admin guard.
The combination of Lines 143-147 treats every non-active submitted status as a disable and still counts inactive admins in sole_admin?. That means a sole admin on trial cannot update unrelated fields, and an active admin can be disabled while only inactive admins remain. app/models/user.rb:35-43 already models trial as enabled via active_or_trial, so this check should only block real transitions into a disabled state and count enabled admins.
Possible fix
def disabling_user?
- user_params.key?(:status) && user_params[:status] != 'active'
+ return false unless user_params.key?(:status)
+
+ new_status = user_params[:status].to_s
+ `@user.status` != new_status && new_status == 'inactive'
end
def sole_admin?
- User.where(admin: true).count == 1
+ User.where(admin: true).active_or_trial.count == 1
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/settings/users_controller.rb` around lines 142 - 147, The
controller currently treats any non-"active" status as a disable and counts all
admins (including inactive/trial) as part of the sole-admin guard; update
disabling_user? to only detect an actual transition into a disabled state by
checking that user_params[:status] is present and the new status is not an
enabled status (e.g. not "active" or "trial" per app/models/user.rb's
active_or_trial semantics), and update sole_admin? to only count enabled admins
(use the active_or_trial scope or method when filtering admins, e.g.
User.active_or_trial.where(admin: true).count == 1) so inactive/trial users are
not treated as enabled admins.
Comment on lines
+56
to
+61
| def point_speed(speed, unit = 'km') | ||
| return speed if speed.to_f <= 0 | ||
|
|
||
| speed * 3.6 | ||
| kmh = speed.to_f * 3.6 | ||
| unit == 'mi' ? (kmh * 0.621371).round(1) : kmh.round(1) | ||
| end |
Contributor
There was a problem hiding this comment.
Convert negative speeds too, or don’t label them as converted units.
speed_label now always renders km/h or mph, but point_speed leaves negative readings in the stored m/s unit. That makes anomalous points display the wrong unit after this change.
Proposed fix
def point_speed(speed, unit = 'km')
- return speed if speed.to_f <= 0
+ return speed if speed.blank?
kmh = speed.to_f * 3.6
- unit == 'mi' ? (kmh * 0.621371).round(1) : kmh.round(1)
+ converted = unit == 'mi' ? kmh * 0.621371 : kmh
+ speed.to_f.zero? ? speed : converted.round(1)
end📝 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.
Suggested change
| def point_speed(speed, unit = 'km') | |
| return speed if speed.to_f <= 0 | |
| speed * 3.6 | |
| kmh = speed.to_f * 3.6 | |
| unit == 'mi' ? (kmh * 0.621371).round(1) : kmh.round(1) | |
| end | |
| def point_speed(speed, unit = 'km') | |
| return speed if speed.blank? | |
| kmh = speed.to_f * 3.6 | |
| converted = unit == 'mi' ? kmh * 0.621371 : kmh | |
| speed.to_f.zero? ? speed : converted.round(1) | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/helpers/application_helper.rb` around lines 56 - 61, point_speed
currently returns the raw value for non-positive speeds, causing mismatch with
speed_label which always shows "km/h" or "mph"; update point_speed (and/or
speed_label usage) so negative/zero readings are converted to the same units as
positives: remove the early "return speed if speed.to_f <= 0" behavior and
always convert using kmh = speed.to_f * 3.6 and the mi branch (unit == 'mi' ?
(kmh * 0.621371).round(1) : kmh.round(1)); alternatively, if you must preserve
raw negatives, make speed_label check for raw-meter-per-second values and avoid
appending "km/h"/"mph" for those cases (reference point_speed and speed_label to
keep behavior consistent).
| 1 # Legacy format | ||
| else | ||
| raise StandardError, 'Unknown export format: neither manifest.json nor data.json found' | ||
| raise UnsupportedFormatError, 'Unknown export format: neither manifest.json nor data.json found' |
Contributor
There was a problem hiding this comment.
Handle unsupported format_version with the same error class.
This branch correctly raises UnsupportedFormatError for missing format markers, but unsupported manifest versions still raise generic StandardError in create_handler (Line 181), causing inconsistent behavior and reporting.
💡 Suggested fix
else
- raise StandardError, "Unsupported export format version: #{format_version}"
+ raise UnsupportedFormatError, "Unsupported export format version: #{format_version}"
end📝 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.
Suggested change
| raise UnsupportedFormatError, 'Unknown export format: neither manifest.json nor data.json found' | |
| else | |
| raise UnsupportedFormatError, "Unsupported export format version: #{format_version}" | |
| end |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/services/users/import_data.rb` at line 170, The code currently raises
UnsupportedFormatError for missing markers but still raises a generic
StandardError in create_handler when encountering an unknown manifest
format_version; update create_handler (the method that parses manifest and
selects handlers) to raise UnsupportedFormatError instead of StandardError for
unsupported or unknown format_version values so error handling is
consistent—replace the generic exception raising logic in create_handler to
raise UnsupportedFormatError with a clear message that includes the offending
format_version and context.
Comment on lines
+51
to
53
| placeholders = supported_sources.map { |s| "'#{s}'" }.join(', ') | ||
| import_ids = execute("SELECT id FROM imports WHERE source IN (#{placeholders})").map { |row| row['id'] } | ||
| return if import_ids.empty? |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== imports.source column type in migrations =="
rg -n -C2 "create_table\\s+:imports|t\\.(integer|string)\\s+:source" db/migrate
echo
echo "== Import enum mapping in model =="
rg -n -C4 "enum\\s+:source" app/models/import.rb
echo
echo "== Raw SQL filter in this migration =="
rg -n -C3 "supported_sources|placeholders|SELECT id FROM imports WHERE source IN" db/migrate/20260125100000_enqueue_transportation_mode_backfill_jobs.rbRepository: Freika/dawarich
Length of output: 2557
Use integer enum values in the raw SQL source filter.
Line 52 builds source IN ('google_semantic_history', ...), but imports.source is an integer-backed enum column (created as t.integer :source in migrations). The database stores integers (0, 1, 2, 3, 6 for the supported sources), not strings, so this query will return no rows. The rescue clause at lines 67–69 silently masks this failure.
Replace the string enum names with their corresponding integer values:
Suggested fix
- placeholders = supported_sources.map { |s| "'#{s}'" }.join(', ')
- import_ids = execute("SELECT id FROM imports WHERE source IN (#{placeholders})").map { |row| row['id'] }
+ supported_source_values = [0, 3, 2, 1, 6] # google_semantic_history, google_phone_takeout, google_records, owntracks, geojson
+ placeholders = supported_source_values.join(', ')
+ import_ids = execute("SELECT id FROM imports WHERE source IN (#{placeholders})").map { |row| row['id'] }📝 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.
Suggested change
| placeholders = supported_sources.map { |s| "'#{s}'" }.join(', ') | |
| import_ids = execute("SELECT id FROM imports WHERE source IN (#{placeholders})").map { |row| row['id'] } | |
| return if import_ids.empty? | |
| supported_source_values = [0, 3, 2, 1, 6] # google_semantic_history, google_phone_takeout, google_records, owntracks, geojson | |
| placeholders = supported_source_values.join(', ') | |
| import_ids = execute("SELECT id FROM imports WHERE source IN (#{placeholders})").map { |row| row['id'] } | |
| return if import_ids.empty? |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db/migrate/20260125100000_enqueue_transportation_mode_backfill_jobs.rb`
around lines 51 - 53, The raw SQL builds a source IN (...) using string enum
names (supported_sources) but imports.source is an integer-backed enum, so
replace the string names with their integer values before building the SQL; map
each element of supported_sources to its corresponding integer (via the model
enum mapping or Import.sources[s]) and use those integers in the execute call
that populates import_ids (the execute("SELECT id FROM imports WHERE source IN
(...)") line) so the query returns rows instead of being silently masked by the
rescue.
Comment on lines
+299
to
+317
| describe 'PATCH /update_registration_settings' do | ||
| before { sign_in admin } | ||
|
|
||
| it 'disables registration' do | ||
| patch update_registration_settings_settings_users_url, | ||
| params: { registration_enabled: '0' } | ||
|
|
||
| expect(response).to redirect_to(settings_users_url) | ||
| expect(DawarichSettings.registration_enabled?).to be false | ||
| end | ||
|
|
||
| it 'enables registration' do | ||
| DawarichSettings.set_registration_enabled(false) | ||
|
|
||
| patch update_registration_settings_settings_users_url, | ||
| params: { registration_enabled: '1' } | ||
|
|
||
| expect(DawarichSettings.registration_enabled?).to be true | ||
| end |
Contributor
There was a problem hiding this comment.
Restore the registration flag after these examples.
DawarichSettings.set_registration_enabled writes through to Rails.cache in config/initializers/03_dawarich_settings.rb:66-69, so these examples leave mutated global state behind for later specs. Save the previous value and restore it in an around block or ensure.
Possible fix
describe 'PATCH /update_registration_settings' do
before { sign_in admin }
+
+ around do |example|
+ previous = DawarichSettings.registration_enabled?
+ example.run
+ ensure
+ DawarichSettings.set_registration_enabled(previous)
+ end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/requests/settings/users_spec.rb` around lines 299 - 317, The tests under
describe 'PATCH /update_registration_settings' mutate global cache via
DawarichSettings.set_registration_enabled and leave state for other specs; wrap
these examples to save and restore the original flag (use
DawarichSettings.registration_enabled? to read current value), for example with
an around block or ensure/finally inside each example so you call
DawarichSettings.set_registration_enabled(previous_value) after the test; update
the examples that call DawarichSettings.set_registration_enabled and the ones
asserting DawarichSettings.registration_enabled? to restore the prior value to
avoid leaking state.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/services/families/locations.rb (1)
32-49:⚠️ Potential issue | 🟠 MajorAdd
.limit(1)and use.includes(:user)to prevent N+1 queries.Line 34 executes a separate query for each sharing member, and lines 39–40 access
point.user.emailtwice without preloading, resulting in 2N+1 queries for N members. Consider refactoring to fetch the latest point per user in a single query with eager-loaded associations:♻️ Suggested optimization using DISTINCT ON
def build_family_locations(sharing_members) - latest_points = - sharing_members.map { _1.points.order(timestamp: :desc).first }.compact + user_ids = sharing_members.map(&:id) + latest_points = Point + .select('DISTINCT ON (user_id) *') + .where(user_id: user_ids) + .order('user_id, timestamp DESC') + .includes(:user) latest_points.map do |point|🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/families/locations.rb` around lines 32 - 49, The method build_family_locations is causing 2N+1 queries; change the latest_points collection to preload users and limit per-user point queries by replacing the current map with something like: iterate over sharing_members.preload(points: :user) (or includes(points: :user)) and for each member call points.order(timestamp: :desc).limit(1).first to fetch a single latest point with its user already loaded; then when building the hash use a local variable for point.user.email to avoid accessing it twice (symbols: build_family_locations, sharing_members, points, user, latest_points).
🧹 Nitpick comments (3)
lib/tasks/points_raw_data.rake (1)
367-370: Intentional but slow:find_each+destroy!for proper callback execution.Using
find_eachwith individualdestroy!calls ensures ActiveStorage callbacks run correctly to purge files. For large datasets this could be slow, but correctness is prioritized here. Consider adding progress output if this is expected to handle many records.♻️ Optional: Add progress indicator for large deletions
# Step 3: Delete all archive records (cascades to ActiveStorage blobs) puts '▸ Step 3/3: Deleting archive records and files...' - Points::RawDataArchive.find_each do |archive| + Points::RawDataArchive.find_each.with_index do |archive, idx| + puts " Deleting archive #{idx + 1}/#{total_archives}..." if (idx + 1) % 100 == 0 archive.file.purge if archive.file.attached? archive.destroy! end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/tasks/points_raw_data.rake` around lines 367 - 370, The current loop over Points::RawDataArchive using find_each with archive.file.purge and archive.destroy! is intentionally correct but can be very slow for many records; update the rake task to add progress output (e.g., a simple counter with periodic processLogger/puts updates or a progress bar) around the find_each iteration so operators can see deletion progress and throughput while preserving the use of archive.file.purge and destroy! to keep ActiveStorage callbacks.spec/tasks/points_raw_data_reset_all_spec.rb (1)
37-65: Missing test coverage for step 1: restoration of cleared points.These tests create points with
raw_data: { 'some' => 'data' }, which meanscleared_pointswill be 0 and step 1 (restoration from archives) is always skipped. Consider adding a test case whereraw_data: {}to exercise the restoration logic.✅ Suggested test for restoration step
context 'when there are cleared points to restore' do let(:user) { create(:user) } let!(:archive) { create(:points_raw_data_archive, user: user) } let!(:point) do create(:point, user: user, raw_data_archived: true, raw_data_archive_id: archive.id, raw_data: {}) end before do allow($stdin).to receive(:gets).and_return("y\n") end it 'restores cleared raw_data from archives' do expect do Rake::Task['points:raw_data:reset_all'].invoke end.to output(/Restoring cleared raw_data from archives/).to_stdout end endBased on learnings: "Test behavior, not implementation" — this suggested test validates the restoration behavior outcome.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/tasks/points_raw_data_reset_all_spec.rb` around lines 37 - 65, Add a new spec context to exercise the restoration branch by creating a point with raw_data: {} (not populated) so cleared_points > 0; in spec/tasks/points_raw_data_reset_all_spec.rb create a context similar to the existing one but set point raw_data to {} and still mark raw_data_archived: true and raw_data_archive_id: archive.id, stub $stdin to "y\n", then invoke the Rake task 'points:raw_data:reset_all' and assert the output includes the restoration message (e.g. /Restoring cleared raw_data from archives/) and that the point's raw_data is restored as expected and the archive record is deleted (Points::RawDataArchive count changes).app/services/families/locations.rb (1)
26-30: Consider moving sharing filter to database query.The
select(&:family_sharing_enabled?)loads all family members into memory before filtering. Iffamily_sharing_enabled?can be represented as a column check, moving it to the SQL query would be more efficient.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/families/locations.rb` around lines 26 - 30, The current family_members_with_sharing_enabled method loads all family members and filters in Ruby via select(&:family_sharing_enabled?), which is inefficient; change it to filter in SQL by replacing the in-memory select with an ActiveRecord where that checks the underlying column (e.g. where(family_sharing_enabled: true)) or, if the predicate is derived logic, add a scope (e.g. scope :with_family_sharing_enabled, -> { where(family_sharing_enabled: true) }) on the User model and call user.family.members.where.not(id: user.id).with_family_sharing_enabled in family_members_with_sharing_enabled so the filtering happens in the database instead of Ruby.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/services/stats/bulk_calculator.rb`:
- Around line 28-36: The SQL construction and array indentation in the
Point.sanitize_sql_array call violate RuboCop: change the double-quoted string
fragments to single-quoted (since there is no interpolation) and reformat the
array so its elements align with the first element per RuboCop's
Layout/FirstArrayElementIndentation rule; update the call around
Point.sanitize_sql_array (and the following Point.connection.select_rows
mapping) to use single-quoted literals for the SQL pieces and properly indent
the array entries (user.timezone, user_id, start_ts, end_ts) so the linter no
longer flags the string-literal and array-indentation offenses.
In `@db/migrate/20260310000002_add_composite_indexes_and_drop_low_selectivity.rb`:
- Around line 6-32: The migration's change method removes a partial index
without specifying the original WHERE clause, so Rollback would recreate a full
index; replace the single change method with explicit up and down methods: in
up, perform the add_index calls for :points and :tracks and remove_index for the
partial index using remove_index :points, name:
:index_points_on_user_id_and_reverse_geocoded_at, algorithm: :concurrently,
if_exists: true (same as current), and in down, re-create the partial index by
calling add_index :points, %i[user_id reverse_geocoded_at], name:
:index_points_on_user_id_and_reverse_geocoded_at, algorithm: :concurrently,
if_not_exists: true, where: "(reverse_geocoded_at IS NOT NULL)" so the original
partial index is restored exactly; keep the other added indexes removed in down
symmetrically.
In `@db/migrate/20260310000003_add_unique_index_to_place_visits.rb`:
- Around line 4-22: The migration's DELETE + concurrent add_index (in up) can
race with app writes because disable_ddl_transaction! lets the delete commit
before the concurrent unique index finishes; to fix, prevent new PlaceVisit
duplicates during the window by either (A) adding a model-level uniqueness
validation on PlaceVisit for [:visit_id, :place_id] plus a short
deployment/feature-flag rollout that blocks create/update of PlaceVisit while
you run the migration, or (B) implement an application-level write guard around
PlaceVisit creations (e.g., temporary feature flag or middleware that
rejects/queues writes) so no inserts can occur between the DELETE and add_index
call; ensure you reference the migration's up method, the DELETE SQL block, and
the add_index :place_visits, %i[visit_id place_id] (name:
:idx_place_visits_visit_id_place_id) when making the change.
In `@lib/tasks/points_raw_data.rake`:
- Around line 356-372: Wrap the entire rake task steps in an
ActiveRecord::Base.transaction block so the reset of Point flags and the
deletion of Points::RawDataArchive records (including archive.file.purge and
archive.destroy!) are atomic; specifically, enclose the code that calls
Point.where(raw_data_archived: true).update_all(...) and the
Points::RawDataArchive.find_each { |archive| archive.file.purge if
archive.file.attached?; archive.destroy! } inside ActiveRecord::Base.transaction
do ... end so any exception during purge/destroy will roll back the flag resets
(and let exceptions bubble up).
---
Outside diff comments:
In `@app/services/families/locations.rb`:
- Around line 32-49: The method build_family_locations is causing 2N+1 queries;
change the latest_points collection to preload users and limit per-user point
queries by replacing the current map with something like: iterate over
sharing_members.preload(points: :user) (or includes(points: :user)) and for each
member call points.order(timestamp: :desc).limit(1).first to fetch a single
latest point with its user already loaded; then when building the hash use a
local variable for point.user.email to avoid accessing it twice (symbols:
build_family_locations, sharing_members, points, user, latest_points).
---
Nitpick comments:
In `@app/services/families/locations.rb`:
- Around line 26-30: The current family_members_with_sharing_enabled method
loads all family members and filters in Ruby via
select(&:family_sharing_enabled?), which is inefficient; change it to filter in
SQL by replacing the in-memory select with an ActiveRecord where that checks the
underlying column (e.g. where(family_sharing_enabled: true)) or, if the
predicate is derived logic, add a scope (e.g. scope
:with_family_sharing_enabled, -> { where(family_sharing_enabled: true) }) on the
User model and call user.family.members.where.not(id:
user.id).with_family_sharing_enabled in family_members_with_sharing_enabled so
the filtering happens in the database instead of Ruby.
In `@lib/tasks/points_raw_data.rake`:
- Around line 367-370: The current loop over Points::RawDataArchive using
find_each with archive.file.purge and archive.destroy! is intentionally correct
but can be very slow for many records; update the rake task to add progress
output (e.g., a simple counter with periodic processLogger/puts updates or a
progress bar) around the find_each iteration so operators can see deletion
progress and throughput while preserving the use of archive.file.purge and
destroy! to keep ActiveStorage callbacks.
In `@spec/tasks/points_raw_data_reset_all_spec.rb`:
- Around line 37-65: Add a new spec context to exercise the restoration branch
by creating a point with raw_data: {} (not populated) so cleared_points > 0; in
spec/tasks/points_raw_data_reset_all_spec.rb create a context similar to the
existing one but set point raw_data to {} and still mark raw_data_archived: true
and raw_data_archive_id: archive.id, stub $stdin to "y\n", then invoke the Rake
task 'points:raw_data:reset_all' and assert the output includes the restoration
message (e.g. /Restoring cleared raw_data from archives/) and that the point's
raw_data is restored as expected and the archive record is deleted
(Points::RawDataArchive count changes).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d82ecbfe-79b2-4572-bdcb-9746aea923a7
📒 Files selected for processing (14)
app/services/areas/visits/create.rbapp/services/families/locations.rbapp/services/places/visits/create.rbapp/services/stats/bulk_calculator.rbapp/services/tracks/build_path.rbapp/services/visits/place_finder.rbdb/migrate/20260310000001_drop_redundant_indexes.rbdb/migrate/20260310000002_add_composite_indexes_and_drop_low_selectivity.rbdb/migrate/20260310000003_add_unique_index_to_place_visits.rbdb/migrate/20260310000006_fix_tracks_original_path_srid.rblib/tasks/points_raw_data.rakespec/services/stats/bulk_calculator_spec.rbspec/services/visits/place_finder_spec.rbspec/tasks/points_raw_data_reset_all_spec.rb
Comment on lines
+4
to
+22
| disable_ddl_transaction! | ||
|
|
||
| def up | ||
| # Remove duplicate (visit_id, place_id) rows, keeping the oldest | ||
| execute <<~SQL.squish | ||
| DELETE FROM place_visits | ||
| WHERE id NOT IN ( | ||
| SELECT MIN(id) | ||
| FROM place_visits | ||
| GROUP BY visit_id, place_id | ||
| ) | ||
| SQL | ||
|
|
||
| # Add unique composite index, replacing both single-column indexes | ||
| add_index :place_visits, %i[visit_id place_id], | ||
| name: :idx_place_visits_visit_id_place_id, | ||
| unique: true, | ||
| algorithm: :concurrently, | ||
| if_not_exists: true |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== PlaceVisit model / validations =="
fd 'place_visit\.rb$' . | xargs -r rg -n -C3 '\bclass\s+PlaceVisit\b|\bvalidates\b.*\buniqueness\b|\bscope:\s*:place_id\b|\bscope:\s*:visit_id\b'
echo
echo "== PlaceVisit write paths =="
rg -n -C2 --type rb '\bPlaceVisit\.(create|create!|create_or_find_by|find_or_create_by|upsert|upsert_all|insert_all)\b|\.place_visits\.(create|create!|create_or_find_by|find_or_create_by)\b'Repository: Freika/dawarich
Length of output: 293
Guard writes before deleting duplicates and building the unique index.
The PlaceVisit model has no uniqueness validation on (visit_id, place_id). With disable_ddl_transaction!, the DELETE at lines 8–15 commits before the concurrent unique index build completes. A concurrent write can reintroduce duplicates during this window, causing the unique index build to fail and leaving the migration half-applied. Either add model-level validation preventing duplicate (visit_id, place_id) pairs, or wrap the migration in application-level write guards to block PlaceVisit inserts during the delete-and-rebuild sequence.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db/migrate/20260310000003_add_unique_index_to_place_visits.rb` around lines 4
- 22, The migration's DELETE + concurrent add_index (in up) can race with app
writes because disable_ddl_transaction! lets the delete commit before the
concurrent unique index finishes; to fix, prevent new PlaceVisit duplicates
during the window by either (A) adding a model-level uniqueness validation on
PlaceVisit for [:visit_id, :place_id] plus a short deployment/feature-flag
rollout that blocks create/update of PlaceVisit while you run the migration, or
(B) implement an application-level write guard around PlaceVisit creations
(e.g., temporary feature flag or middleware that rejects/queues writes) so no
inserts can occur between the DELETE and add_index call; ensure you reference
the migration's up method, the DELETE SQL block, and the add_index
:place_visits, %i[visit_id place_id] (name: :idx_place_visits_visit_id_place_id)
when making the change.
Comment on lines
+356
to
+372
| # Step 2: Reset archival flags on all points | ||
| puts '▸ Step 2/3: Resetting archival flags on points...' | ||
| reset_count = Point.where(raw_data_archived: true).update_all( | ||
| raw_data_archived: false, | ||
| raw_data_archive_id: nil | ||
| ) | ||
| puts " Reset #{reset_count} points." | ||
| puts '' | ||
|
|
||
| # Step 3: Delete all archive records (cascades to ActiveStorage blobs) | ||
| puts '▸ Step 3/3: Deleting archive records and files...' | ||
| Points::RawDataArchive.find_each do |archive| | ||
| archive.file.purge if archive.file.attached? | ||
| archive.destroy! | ||
| end | ||
| puts " Deleted #{total_archives} archive records." | ||
| puts '' |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if similar patterns exist in other rake tasks in this file
# and if there's any transaction wrapping precedent
rg -n "transaction|ActiveRecord::Base.transaction" lib/tasks/Repository: Freika/dawarich
Length of output: 121
🏁 Script executed:
cat -n lib/tasks/points_raw_data.rake | head -400 | tail -100Repository: Freika/dawarich
Length of output: 4317
Wrap the task in a transaction to prevent data loss on partial failure.
If step 2 (flag reset) succeeds but step 3 (archive deletion) fails partway through, re-running the task will skip step 1 because cleared_points will be recalculated as 0 after the flags are already reset. This leaves points with empty raw_data unrecoverable. Wrapping all three steps in ActiveRecord::Base.transaction do...end ensures atomicity or complete rollback on any failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@lib/tasks/points_raw_data.rake` around lines 356 - 372, Wrap the entire rake
task steps in an ActiveRecord::Base.transaction block so the reset of Point
flags and the deletion of Points::RawDataArchive records (including
archive.file.purge and archive.destroy!) are atomic; specifically, enclose the
code that calls Point.where(raw_data_archived: true).update_all(...) and the
Points::RawDataArchive.find_each { |archive| archive.file.purge if
archive.file.attached?; archive.destroy! } inside ActiveRecord::Base.transaction
do ... end so any exception during purge/destroy will roll back the flag resets
(and let exceptions bubble up).
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
API Changes
Chores
Tests