Conversation
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Bumps [database_consistency](https://github.com/djezzzl/database_consistency) from 2.0.6 to 3.0.0. - [Changelog](https://github.com/djezzzl/database_consistency/blob/master/CHANGELOG.md) - [Commits](djezzzl/database_consistency@v2.0.6...v3.0.0) --- updated-dependencies: - dependency-name: database_consistency dependency-version: 3.0.0 dependency-type: direct:development update-type: version-update:semver-major ... Signed-off-by: dependabot[bot] <support@github.com>
Add API endpoints for importing files (GPX, GeoJSON, KML, OwnTracks, etc.) using API key authentication. This enables external tools and integrations to upload files without requiring browser-based session auth, which is essential for OIDC-configured instances. Endpoints: - POST /api/v1/imports - Upload a file for import (multipart/form-data) - GET /api/v1/imports - List user's imports (paginated) - GET /api/v1/imports/:id - Show import status and details The create action accepts a multipart file upload, creates an Active Storage blob, and saves an Import record. The existing after_commit callback automatically enqueues Import::ProcessJob for background processing. Source type is auto-detected by Imports::SourceDetector.
- Catch ActiveRecord::RecordInvalid separately with user-friendly message - Return generic message for unexpected StandardError (avoid leaking SQL/internals) - Return 500 instead of 422 for unexpected errors - Cap per_page at 100 to prevent abuse
Bumps [omniauth-google-oauth2](https://github.com/zquestz/omniauth-google-oauth2) from 1.2.1 to 1.2.2. - [Release notes](https://github.com/zquestz/omniauth-google-oauth2/releases) - [Changelog](https://github.com/zquestz/omniauth-google-oauth2/blob/master/CHANGELOG.md) - [Commits](zquestz/omniauth-google-oauth2@v1.2.1...v1.2.2) --- updated-dependencies: - dependency-name: omniauth-google-oauth2 dependency-version: 1.2.2 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] <support@github.com>
…istency-3.0.0 Bump database_consistency from 2.0.6 to 3.0.0
…le-oauth2-1.2.2 Bump omniauth-google-oauth2 from 1.2.1 to 1.2.2
mobile layout optimizations
Fix LocationIQ support
feat(api): add file import endpoints (POST/GET /api/v1/imports)
Fixed IDL crossing render issue
Implement family location history viewing along with some fixes
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds server-persisted onboarding and a multi-screen onboarding modal; implements family location-request workflow (model, controller, mailer, views, expiry job); exposes family location history API with frontend history rendering; adds Imports API and UI; IDL-aware route segmentation; multiple data migrations and extensive tests. Changes
Sequence Diagram(s)sequenceDiagram
actor Requester
participant FamilyCtrl as Family::LocationRequestsController
participant CreateService as Families::CreateLocationRequest
participant DB as Database
participant Notifier as Notification Service
participant Mailer as Email Job
Requester->>FamilyCtrl: POST /family/location_requests
FamilyCtrl->>CreateService: initialize(requester, target_user).call
CreateService->>DB: verify family membership & sharing state
CreateService->>DB: check cooldown & create LocationRequest
CreateService->>Notifier: create in-app notification
CreateService->>Mailer: enqueue FamilyMailer.location_request
CreateService-->>FamilyCtrl: Result (created or error)
FamilyCtrl-->>Requester: Redirect/response with flash
sequenceDiagram
actor User
participant MapCtrl as MapLibre Controller
participant API as Api::V1::Families::LocationsController
participant DB as Database
participant FamilyLayer as FamilyLayer
participant UI as Map UI
User->>MapCtrl: open map / change month
MapCtrl->>MapCtrl: debouncedLoadFamilyHistory()
MapCtrl->>API: GET /api/v1/families/locations/history?start_at&end_at
API->>DB: query sharing members and points (sample if needed)
API-->>MapCtrl: per-member history payload
MapCtrl->>FamilyLayer: loadMemberHistory(historyData)
FamilyLayer-->>UI: render history polylines
MapCtrl->>UI: update member info lines
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
app/controllers/settings/background_jobs_controller.rb (1)
29-29:⚠️ Potential issue | 🟡 MinorRemove dead
flash.nowassignment.
flash.now[:notice]only persists for the current request and is lost on redirect. Since line 39 already passesnotice:toredirect_to, this line has no effect and should be removed.🧹 Proposed fix
def create EnqueueBackgroundJob.perform_later(params[:job_name], current_user.id) - flash.now[:notice] = 'Job was successfully created.' - redirect_path = case params[:job_name] when 'start_immich_import', 'start_photoprism_import'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/settings/background_jobs_controller.rb` at line 29, Remove the redundant flash.now assignment: delete the line setting flash.now[:notice] = 'Job was successfully created.' in the BackgroundJobsController action (the one that later calls redirect_to ... , notice: ...). The redirect_to already supplies the persistent notice via the notice: option, so remove the flash.now[:notice] statement to eliminate the dead assignment.spec/requests/settings/background_jobs_spec.rb (1)
47-50:⚠️ Potential issue | 🟠 MajorTest uses wrong HTTP method and has no assertion.
This test uses
getinstead ofpostfor the create action, and lacks any expectation. Compare to thestart_immich_importtest at lines 34-38 which correctly usespostand asserts the redirect.🐛 Proposed fix
context 'when job name is start_photoprism_import' do it 'redirects to imports page' do - get settings_background_jobs_url, params: { job_name: 'start_photoprism_import' } + post settings_background_jobs_url, params: { job_name: 'start_photoprism_import' } + + expect(response).to redirect_to(imports_url) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/requests/settings/background_jobs_spec.rb` around lines 47 - 50, Change the test for job_name 'start_photoprism_import' to use POST (replace get with post) against settings_background_jobs_url with params: { job_name: 'start_photoprism_import' } and add an assertion that the response redirects to the imports page (mirror the start_immich_import example); e.g. after calling post expect(response).to redirect_to(settings_imports_url) so the test both exercises the create action and verifies the redirect.app/services/stats/calculate_month.rb (1)
26-30:⚠️ Potential issue | 🟠 MajorMonth boundary still excludes most of the last day.
At Line 29,
DateTime.new(year, month, -1).to_iresolves to00:00:00of the last day, so points later that day are excluded from monthly distance/toponyms.💡 Proposed fix
def end_timestamp - DateTime.new(year, month, -1).to_i + DateTime.new(year, month, -1, 23, 59, 59).to_i end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/stats/calculate_month.rb` around lines 26 - 30, The end_timestamp currently builds a DateTime for the last day with time 00:00:00, excluding events later that day; update the end_timestamp logic in calculate_month.rb (the end_timestamp method) to return the timestamp for the very end of the last day of the month (for example by constructing the last-day DateTime with time 23:59:59 or by taking the next day at 00:00:00 and subtracting one second) so that points occurring any time on the final day are included.
🟡 Minor comments (10)
db/migrate/20260314000001_fix_route_opacity_default.rb-5-5 (1)
5-5:⚠️ Potential issue | 🟡 MinorEdge case: value of exactly
1is ambiguous.A
route_opacityvalue of1won't be converted (condition is> 1), but it could represent either 1% on the old scale or 100% on the new scale. If any users have this exact value stored, it will be interpreted as full opacity (100%) instead of 1%. This is likely a rare edge case, but worth noting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/migrate/20260314000001_fix_route_opacity_default.rb` at line 5, Change the selection to include values equal to 1 and handle that edge explicitly: update the query in User.where("(settings->>'route_opacity')::float >= 1") to fetch those records, and inside the processing loop (the code that reads settings['route_opacity'] and rewrites it) add a branch for when the numeric value == 1 that makes the intended conversion explicit (e.g. map 1 -> 0.01) or logs/flags the user for manual review; ensure you reference and update the same settings['route_opacity'] field consistently and add a comment explaining why 1 is treated specially.spec/helpers/stats_helper_spec.rb-15-17 (1)
15-17:⚠️ Potential issue | 🟡 Minor“Known country variant” example does not use a variant input.
The test name says variant normalization, but Line 16 passes
"Tanzania"(already canonical). Use an alias (e.g.,"United Republic of Tanzania") to validate the mapping path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/helpers/stats_helper_spec.rb` around lines 15 - 17, The test currently claims to validate variant normalization but passes a canonical name; update the spec to pass a known alias (e.g., "United Republic of Tanzania") to trigger the mapping path in normalize_country_name and assert the result equals 'Tanzania' so the test actually verifies variant normalization for the normalize_country_name helper.app/services/countries_and_cities.rb-15-17 (1)
15-17:⚠️ Potential issue | 🟡 MinorCountry canonicalization is bypassed when
country_nameis nil.The filter at Line 15 removes points before
canonical_country_name(point)can usecountry_id, so valid points can be dropped.💡 Proposed fix
def call points - .reject { |point| point[:country_name].nil? || point[:city].nil? } + .reject { |point| point[:city].nil? } .group_by { |point| canonical_country_name(point) } + .reject { |country, _| country.blank? } .transform_values { |country_points| process_country_points(country_points) } .map { |country, cities| CountryData.new(country: country, cities: cities) } end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/services/countries_and_cities.rb` around lines 15 - 17, The current reject removes any point with a nil :country_name before canonical_country_name(point) can fallback to :country_id, causing valid points to be dropped; update the filter to only reject when both :country_name and :country_id are missing (e.g. replace .reject { |point| point[:country_name].nil? } with .reject { |point| point[:country_name].nil? && point[:country_id].nil? }), or alternatively make canonical_country_name(point) robust by falling back to point[:country_id] when country_name is nil so grouping (.group_by { |point| canonical_country_name(point) }) preserves points for process_country_points.app/javascript/controllers/onboarding_modal_controller.js-37-39 (1)
37-39:⚠️ Potential issue | 🟡 MinorMove the modal
closehook to Stimulus markup or clean it up explicitly.
checkAndShowModal()adds a new anonymouscloselistener every time it shows the dialog, and nothing removes that handler indisconnect(). Reconnects/reopens can therefore firecompleteOnboarding()more than once.As per coding guidelines, "Always clean up in
disconnect()and preferdata-actionattributes in HTML overaddEventListener."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/controllers/onboarding_modal_controller.js` around lines 37 - 39, The anonymous `close` listener added in checkAndShowModal causes duplicate calls to completeOnboarding because it's never removed; either move the handler to Stimulus markup using a data-action on the dialog (e.g. map the dialog's close event to onboarding_modal_controller#completeOnboarding) or replace the anonymous listener with a bound instance method (store it as this.onModalClose = this.completeOnboarding.bind(this) when adding in checkAndShowModal) and remove it in disconnect() (this.modalTarget.removeEventListener("close", this.onModalClose)); update references to modalTarget, checkAndShowModal, completeOnboarding, and disconnect accordingly.e2e/v2/map/layers/family.spec.js-723-740 (1)
723-740:⚠️ Potential issue | 🟡 MinorPrefer stable test hooks over controller internals and
source._data.These assertions reach through
[data-maps--maplibre-target],window.Stimulus, and private MapLibre source internals. They'll break on harmless refactors even when the user-visible behavior is unchanged. Expose a helper ordata-testidfor the history state you want to assert instead.As per coding guidelines, "e2e/**/*.{js,ts}: Use
data-testidselectors in browser test files for DOM queries".Also applies to: 792-813, 865-892, 949-961
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/v2/map/layers/family.spec.js` around lines 723 - 740, This test reaches into controller internals and MapLibre private properties (querying '[data-controller*="maps--maplibre"]', window.Stimulus/Application, controller for "maps--maplibre", layer "family-history" and source "family-source-history" and source._data.features) which is fragile; instead add a stable test hook (e.g. a data-testid attribute or a small helper on the DOM that exposes history state) in the map component and update the spec to read that testid (e.g. data-testid="family-history-state") to assert hasLayer/hasSource/featureCount; change the component rendering code that initializes the map controller to set this testid (or updateMapHistoryState helper) whenever the history layer/source changes so tests query DOM via document.querySelector('[data-testid="family-history-state"]') rather than accessing controller.map or source._data.app/javascript/maps_maplibre/utils/route_segmenter.js-120-140 (1)
120-140:⚠️ Potential issue | 🟡 MinorKeep
pointCounttied to source points.
unwrapCoordinates()now injects synthetic ±180° vertices for antimeridian stitching, socoordinates.flat().lengthovercounts the original GPS samples whenever a route becomesMultiLineString. IfpointCountis meant to describe input points, it should staysegment.length.Suggested fix
- pointCount: isMultiPath ? coordinates.flat().length : segment.length, + pointCount: segment.length,Also applies to: 236-240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/maps_maplibre/utils/route_segmenter.js` around lines 120 - 140, The code currently computes pointCount from coordinates.flat().length which counts injected ±180° stitch vertices; change it so pointCount reflects the original source points by using segment.length (or the original per-segment length variable) instead of the flattened coordinates count; locate this logic near unwrapCoordinates / coordsList handling and where pointCount is set (also update the same pattern around the second occurrence referenced at the other block, e.g., lines handling coordsList at 236-240) so any UI/stats refer to the original GPS sample count rather than the stitched vertex count.app/views/family/location_requests/show.html.erb-17-17 (1)
17-17:⚠️ Potential issue | 🟡 MinorInconsistent expiry logic between conditions.
Line 17 checks
@request.pending? &&@request.expires_at> Time.current, while Line 46 checks@request.expired? ||@request.expires_at<= Time.current. This creates potential inconsistency:
- A request could be
pendingin database but time-expired (if background job hasn't updated status yet)- The dual check suggests the
expired?status isn't reliably synchronized withexpires_atConsider consolidating the expiry check into a single method on the model (e.g.,
actually_expired?oractionable?) to ensure consistent behavior.💡 Suggested model method
# In Family::LocationRequest model def actionable? pending? && expires_at > Time.current end def effectively_expired? expired? || expires_at <= Time.current endThen in view:
<% if `@request.actionable`? %> <!-- show accept/decline form --> <% else %> <% if `@request.effectively_expired`? %>Also applies to: 46-46
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/family/location_requests/show.html.erb` at line 17, Consolidate the expiry/availability logic into the Family::LocationRequest model to avoid inconsistent checks in the view: add a predicate like actionable? (returning pending? && expires_at > Time.current) and effectively_expired? (returning expired? || expires_at <= Time.current), then update the view conditionals that currently reference `@request.pending`? && `@request.expires_at` > Time.current and `@request.expired`? || `@request.expires_at` <= Time.current to use `@request.actionable`? and `@request.effectively_expired`? respectively so all expiry logic is centralized and consistent.spec/requests/family/location_requests_spec.rb-49-56 (1)
49-56:⚠️ Potential issue | 🟡 MinorAvoid silently swallowing exceptions in tests.
The
rescue ActionView::Template::Errorblock masks real failures and always passes withexpect(true).to be true. If there's a known view issue, consider:
- Skipping the test with
skip "pending icon fix"- Mocking the problematic icon helper
- Fixing the root cause
This pattern can hide regressions and makes test results misleading.
Proposed fix using skip
it 'shows the request detail page' do - get family_location_request_path(request_record) - expect(response).to have_http_status(:ok) - rescue ActionView::Template::Error - # Pre-existing icon issue in application layout - skip template rendering check - # The controller action itself works correctly (no redirect = 200 intent) - expect(true).to be true + skip "Pending icon fix in application layout" if ENV['SKIP_ICON_TESTS'] + get family_location_request_path(request_record) + expect(response).to have_http_status(:ok) end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/requests/family/location_requests_spec.rb` around lines 49 - 56, The test "shows the request detail page" rescues ActionView::Template::Error and masks failures; instead remove the rescue and either mark the example pending with skip "pending icon fix" or mock/stub the failing view helper that renders the icon so the controller behavior is still asserted; locate the example that calls get family_location_request_path(request_record) and replace the rescue block with a skip call or add a stub for the problematic icon helper (or fix the icon) so the test fails properly on real errors rather than always passing.swagger/v1/swagger.yaml-874-880 (1)
874-880:⚠️ Potential issue | 🟡 MinorFix invalid OpenAPI 3.0 schema for file upload.
type: fileis not valid in OpenAPI 3.0. Usetype: objectwith apropertiesobject containing the file parameter astype: stringwithformat: binary.Proposed fix
requestBody: content: multipart/form-data: schema: - type: file + type: object + properties: + file: + type: string + format: binary + required: + - file required: true description: The file to import (GPX, GeoJSON, KML, OwnTracks JSON, etc.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@swagger/v1/swagger.yaml` around lines 874 - 880, Replace the invalid OpenAPI 3.0 `type: file` schema under the operation's `requestBody`/`multipart/form-data` block with a proper object schema: set `schema.type: object`, add a `properties` map containing a `file` property defined as `type: string` and `format: binary`, and mark the file field as required (e.g., `required: ["file"]`); keep the existing description and `required: true` at the `requestBody` level or remove the top-level `required` in favor of the schema's `required` array to conform to OpenAPI 3.0.app/javascript/controllers/maps/maplibre_controller.js-1263-1285 (1)
1263-1285:⚠️ Potential issue | 🟡 MinorReset stale member history text before rendering the new payload.
When the next response is empty, omits a member, or omits
sharing_since, the oldSharing since ...line stays in the sidebar even though the layer data was cleared/replaced.🧹 Minimal fix
updateFamilyInfoLines(historyMembers) { if (!this.hasFamilyMembersContainerTarget) return + for (const infoEl of this.familyMembersContainerTarget.querySelectorAll("[data-member-info]")) { + infoEl.textContent = "" + } + for (const member of historyMembers) { const infoEl = this.familyMembersContainerTarget.querySelector( `[data-member-info="${member.user_id}"]`, ) if (!infoEl || !member.sharing_since) continue ... } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/controllers/maps/maplibre_controller.js` around lines 1263 - 1285, Clear stale "Sharing since" text before applying the new payload: in updateFamilyInfoLines (and using familyMembersContainerTarget and data-member-info elements), first iterate all elements matching `[data-member-info]` inside familyMembersContainerTarget and clear their textContent (or the specific sharing line) so previous messages are removed; then continue the existing loop over historyMembers and only set infoEl.textContent when member.sharing_since is present (using the same infoEl lookup and formatting logic) so omitted or empty responses don't leave old text visible.
🧹 Nitpick comments (18)
spec/services/users/safe_settings_spec.rb (1)
72-72: Consider updating custom test values to use the new 0-1 scale.The custom value tests still use
80forroute_opacity, which is on the old 0-100 scale. While the code will accept any numeric value, using a value like0.8would be more consistent with the new default (0.6) and better represent realistic user input going forward.This is a minor consistency concern and doesn't affect test correctness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/services/users/safe_settings_spec.rb` at line 72, Update the custom test value for the 'route_opacity' setting in the spec (safe_settings_spec.rb) from the old 0-100 scale to the new 0-1 scale by replacing the numeric literal 80 with 0.8 wherever the test builds the custom settings hash (the key 'route_opacity' in the example response/custom settings used by the spec); no other logic changes are required, just use 0.8 to match the new default scale of 0.6 and maintain consistency.db/migrate/20260314000001_fix_route_opacity_default.rb (1)
4-11: Consider a single SQL UPDATE for better performance.Row-by-row iteration with
find_eachandsave!may be slow on large datasets. If model callbacks/validations aren't required, a single SQL update would be more efficient:♻️ Suggested SQL-based approach
def up - User.where("(settings->>'route_opacity')::float > 1").find_each do |user| - old_value = user.settings['route_opacity'].to_f - new_value = old_value / 100.0 - user.settings = user.settings.merge('route_opacity' => new_value) - user.save! - end + execute <<~SQL + UPDATE users + SET settings = jsonb_set( + settings, + '{route_opacity}', + to_jsonb((settings->>'route_opacity')::float / 100.0) + ) + WHERE (settings->>'route_opacity')::float > 1 + SQL endIf callbacks or validations are intentionally needed, the current approach is acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@db/migrate/20260314000001_fix_route_opacity_default.rb` around lines 4 - 11, The migration's up method currently iterates User.where(...).find_each and saves each record, which is slow; replace the row-by-row loop in the up method with a single SQL UPDATE executed via the DB connection: update the users.settings jsonb key "route_opacity" by dividing (settings->>'route_opacity')::float by 100 for rows where that expression > 1, using ActiveRecord::Base.connection.execute (or similar) instead of User.find_each/save! so callbacks/validations are skipped and the operation runs as one set-based SQL statement.app/views/devise/registrations/edit.html.erb (2)
18-18: Consider enabling Turbo for the profile form.The form has
turbo: false, which disables Turbo and triggers a full page reload on submission. If this is intentional for Devise compatibility, consider adding a comment explaining why. Otherwise, removingturbo: falsewould align with the "Turbo first" guideline for smoother in-place updates.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/devise/registrations/edit.html.erb` at line 18, The profile edit form uses form_for(...) with data: { turbo_method: :put, turbo: false } which disables Turbo and causes a full page reload; either remove the turbo: false flag from the form_for call to allow Turbo-driven in-place submission, or if disabling Turbo is intentional for Devise compatibility, add an inline comment next to the form_for invocation explaining why Turbo is disabled (mentioning Devise/CSRF/Turbo conflict or specific bug) so future readers know this is deliberate.
173-173: Redundantmethod: :deleteattribute.The link has both
method: :deleteanddata: { turbo_method: :delete }. With Turbo enabled, onlyturbo_method: :deleteis needed. Themethod: :deleteis a rails-ujs fallback that's unnecessary here.Proposed fix
- <%= link_to "Cancel my account", registration_path(resource_name), data: { turbo_confirm: "Are you sure?", turbo_method: :delete }, method: :delete, class: 'btn btn-error btn-outline w-full sm:w-auto' %> + <%= link_to "Cancel my account", registration_path(resource_name), data: { turbo_confirm: "Are you sure?", turbo_method: :delete }, class: 'btn btn-error btn-outline w-full sm:w-auto' %>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/devise/registrations/edit.html.erb` at line 173, Remove the redundant Rails UJS fallback by deleting the method: :delete option from the link_to call that renders the "Cancel my account" button (the call that uses registration_path(resource_name) and data: { turbo_confirm: "Are you sure?", turbo_method: :delete }); keep the data: { turbo_method: :delete } and other attributes (e.g., class) intact so Turbo handles the DELETE request.app/services/reverse_geocoding/places/fetch_data.rb (1)
194-215: Consider handling the edge case where data lacks bothgeometryandlat/lon.If data passes through unchanged (line 196), downstream access to
data['geometry']['coordinates'](line 41) will raise aNoMethodError. While this scenario is unlikely with known geocoder providers, you may want to returnnilor raise a descriptive error for unexpected formats to fail fast with clarity.💡 Optional defensive handling
def normalize_geocoder_data(data) return data if data.key?('geometry') - return data unless data['lat'] && data['lon'] + unless data['lat'] && data['lon'] + Rails.logger.warn("Unexpected geocoder data format: #{data.keys.join(', ')}") + return nil + end address = data['address'] || {}Then handle
nilin the caller:def update_place(reverse_geocoded_place) return if reverse_geocoded_place.nil? data = normalize_geocoder_data(reverse_geocoded_place.data) + return if data.nil?🤖 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 194 - 215, normalize_geocoder_data currently returns the original data when it has neither 'geometry' nor 'lat'/'lon', which can cause a NoMethodError later when callers access data['geometry']['coordinates']; change normalize_geocoder_data to explicitly fail-fast by either returning nil or raising a descriptive error (e.g., "Invalid geocoder data: missing geometry and lat/lon") when neither 'geometry' nor both 'lat' and 'lon' are present, and update callers that expect geometry (places that read data['geometry']['coordinates']) to handle the nil/exception accordingly; refer to the normalize_geocoder_data method and the places that access data['geometry']['coordinates'] to implement this defensive behavior.spec/services/reverse_geocoding/places/fetch_data_spec.rb (1)
663-684: Minor improvement: Add assertion for the type suffix in name.The test verifies the name includes "123 Main Street" but doesn't verify the type suffix formatting. Consider adding an assertion to ensure the full name format is correct.
💡 More complete assertion
place.reload - expect(place.name).to include('123 Main Street') + expect(place.name).to eq('123 Main Street (House)')🤖 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 663 - 684, The test should also assert the type suffix is appended to the place name; after calling service.call and place.reload, verify place.name includes the display_name first part plus the type in the expected format (e.g., "123 Main Street (house)") so update the expectation that currently uses expect(place.name).to include('123 Main Street') to instead assert the full formatted name using the existing nominatim_without_type_key (with 'type' => 'house'), service.call, and place.reload symbols to locate the code.spec/requests/visits_spec.rb (1)
123-128: Also assert the model outcome in the non-status update test.Right now this can pass on response shape alone; add a persisted value assertion so it validates behavior, not just stream wiring.
Suggested test improvement
it 'returns turbo_stream replace on non-status update' do patch visit_url(visit), params: { visit: { name: 'New Name' } }, as: :turbo_stream + expect(visit.reload.name).to eq('New Name') expect_turbo_stream_response expect_turbo_stream_action('replace', "visit_name_#{visit.id}") endAs per coding guidelines "spec/**/*.rb: Test behavior, not implementation... don't test wiring without outcomes."
🤖 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 123 - 128, The test 'returns turbo_stream replace on non-status update' currently only asserts the turbo_stream response; update it to also assert the persisted model change by reloading the Visit and checking its attribute (e.g., assert visit.reload.name equals 'New Name') after calling patch visit_url(visit) so the spec verifies behavior not just response wiring; locate the example in spec/requests/visits_spec.rb around the example block and add the persisted value assertion following the request and before or after the existing expect_turbo_stream_* expectations.app/views/settings/_navigation.html.erb (1)
7-7: Inconsistent active tab detection pattern.This line uses
controller_path == 'settings/users'while all other tabs use theactive_tab?helper. Consider usingactive_tab?(settings_users_path)for consistency, unless there's a specific reason the helper doesn't work correctly for this route (e.g., nested resources).♻️ Suggested consistency fix
- <%= link_to 'Users', settings_users_path, role: 'tab', class: "tab tab-lg #{controller_path == 'settings/users' ? 'tab-active' : ''}" %> + <%= link_to 'Users', settings_users_path, role: 'tab', class: "tab tab-lg #{active_tab?(settings_users_path)}" %>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/settings/_navigation.html.erb` at line 7, Replace the inline controller_path equality check with the shared helper for consistent active-tab detection: change the conditional in the Users tab link (the link_to call that currently uses controller_path == 'settings/users') to use active_tab?(settings_users_path) so it matches other tabs; if the helper does not correctly detect this route due to nested resources, update the active_tab? implementation to handle settings_users_path or add a brief comment explaining the exception.app/javascript/controllers/family_navbar_indicator_controller.js (1)
24-32: Pre-existing issue: Event listener removal won't work due to.bind(this).The
disconnect()method uses.bind(this)which creates new function references different from those added inconnect(). The listeners will never actually be removed.♻️ Suggested fix to properly remove event listeners
+ connect() { + console.log("Family navbar indicator controller connected") + this.updateIndicator() + + // Bind handlers once and store references + this.boundHandleSharingUpdate = this.handleSharingUpdate.bind(this) + this.boundHandleSharingExpired = this.handleSharingExpired.bind(this) + + document.addEventListener( + "location-sharing:updated", + this.boundHandleSharingUpdate, + ) + document.addEventListener( + "location-sharing:expired", + this.boundHandleSharingExpired, + ) + } + disconnect() { document.removeEventListener( "location-sharing:updated", - this.handleSharingUpdate.bind(this), + this.boundHandleSharingUpdate, ) document.removeEventListener( "location-sharing:expired", - this.handleSharingExpired.bind(this), + this.boundHandleSharingExpired, ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/controllers/family_navbar_indicator_controller.js` around lines 24 - 32, disconnect() is currently calling removeEventListener with this.handleSharingUpdate.bind(this) and this.handleSharingExpired.bind(this), which creates new function references so the original listeners (added in connect()) are not removed; fix by storing the bound handlers as persistent properties when you add them (for example set this.handleSharingUpdateBound = this.handleSharingUpdate.bind(this) and this.handleSharingExpiredBound = this.handleSharingExpired.bind(this) in connect() or the constructor) and then call removeEventListener with those same stored references in disconnect() so the listeners are actually removed.spec/requests/api/v1/families/locations_spec.rb (2)
102-109: Consider adding edge case tests for invalid date formats.The test covers missing
start_at/end_atparams, but the controller likely needs to handle malformed date strings as well. Consider adding a test for invalid date format handling.💡 Suggested additional test case
context 'with invalid date format' do it 'returns bad request' do get '/api/v1/families/locations/history', params: { api_key: user.api_key, start_at: 'invalid', end_at: 'also-invalid' } expect(response).to have_http_status(:bad_request) end end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/requests/api/v1/families/locations_spec.rb` around lines 102 - 109, Add a new spec context to cover malformed date strings: in the same spec that contains the context 'without start_at or end_at' add a new context 'with invalid date format' which issues a GET to '/api/v1/families/locations/history' with params including api_key plus start_at: 'invalid' and end_at: 'also-invalid' and assert expect(response).to have_http_status(:bad_request); this mirrors the existing test pattern and ensures the controller properly returns 400 for malformed start_at/end_at params.
76-80: Direct settings manipulation is fragile.Directly modifying the nested
settingshash viadeep_mergecouples this test to the internal settings structure. If the schema changes, these tests will silently break or become incorrect.Consider extracting a helper method or using the same service/method that production code uses to set
started_at.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/requests/api/v1/families/locations_spec.rb` around lines 76 - 80, Replace the fragile direct deep_merge of other_user.settings with the same public API used in production or a small test helper: instead of calling other_user.update!(settings: other_user.settings.deep_merge(...)), call the service/method that sets family location sharing (e.g. Users::SettingsUpdater.update(other_user, family: { location_sharing: { started_at: 1.week.ago.iso8601 } }) or a test helper like set_user_setting(other_user, 'family.location_sharing.started_at', 1.week.ago.iso8601)); this keeps the test resilient to internal settings shape changes and centralizes update logic for other_user and its settings.app/models/family/membership.rb (1)
25-36: Consider atomicity of cleanup operations.If
update_family_location_sharing!succeeds but theupdate_allfor location requests fails (or vice versa), the system could be left in an inconsistent state. Consider wrapping both operations in a transaction.Additionally, the broad
StandardErrorrescue will silently swallow database errors. Consider logging the specific operation that failed.♻️ Suggested improvement
def cleanup_on_departure - # Disable location sharing for departing user - user.update_family_location_sharing!(false) if user.family_sharing_enabled? - - # Expire all pending location requests involving the departing user - Family::LocationRequest - .pending - .where('requester_id = ? OR target_user_id = ?', user_id, user_id) - .update_all(status: :expired, updated_at: Time.current) + ActiveRecord::Base.transaction do + user.update_family_location_sharing!(false) if user.family_sharing_enabled? + + Family::LocationRequest + .pending + .where('requester_id = ? OR target_user_id = ?', user_id, user_id) + .update_all(status: Family::LocationRequest.statuses[:expired], updated_at: Time.current) + end rescue StandardError => e ExceptionReporter.call(e, "Error cleaning up on family departure: #{e.message}") end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/family/membership.rb` around lines 25 - 36, Wrap both operations in a single ActiveRecord transaction inside cleanup_on_departure so they succeed or fail atomically: call user.update_family_location_sharing!(false) and the Family::LocationRequest.pending...update_all inside ActiveRecord::Base.transaction (or transaction on the model), and remove the broad silent rescue; instead rescue ActiveRecord::ActiveRecordError (or StandardError if you must) and include which operation failed in the ExceptionReporter.call message (referencing cleanup_on_departure, update_family_location_sharing! and the Family::LocationRequest.update_all operation), then re-raise the exception so failures are not silently swallowed.spec/mailers/family_mailer_spec.rb (1)
30-32: Consider also testing the plain-text body.Given that both HTML and text templates exist (
location_request.html.erbandlocation_request.text.erb), consider adding a test for the text body to ensure both multipart versions render correctly.💡 Suggested additional test
it 'renders the text body with the request url' do expect(mail.text_part.body.encoded).to include('View Request') end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/mailers/family_mailer_spec.rb` around lines 30 - 32, The spec currently only asserts the HTML part; add a parallel expectation that the plain-text part renders by asserting mail.text_part.body.encoded includes the expected string (e.g., 'View Request'), so update the example spec (the it block for rendering bodies) to also check mail.text_part.body.encoded contains the request URL/link text and ensure the mail object has a text_part before asserting.app/views/shared/_navbar.html.erb (1)
83-88: Apply the active class to the link, not the wrapperThe active state is currently set on a wrapper
<div>, which can make nav highlighting inconsistent. Putactive_class?(family_path)on the<a>(link_to) class directly.As per coding guidelines: `app/**/*.{css,scss,html,erb}: Use Tailwind + DaisyUI for styling.`Proposed fix
- <div class="<%= active_class?(family_path) %>"> - <%= link_to family_path, class: "mx-1 flex items-center space-x-2" do %> + <%= link_to family_path, class: "mx-1 #{active_class?(family_path)} flex items-center space-x-2" do %> <span>Family<sup>α</sup></span> <%= render 'families/navbar_indicator', user: current_user %> <% end %> - </div>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/views/shared/_navbar.html.erb` around lines 83 - 88, Move the active_class?(family_path) call from the wrapper <div> to the <a> generated by link_to: remove the class attribute on the outer div and add active_class?(family_path) into the link_to's class list (merging it with "mx-1 flex items-center space-x-2"), so the anchor receives the Tailwind/DaisyUI active styling; update the file fragment around link_to and the wrapper div (referencing active_class?(family_path) and link_to) accordingly.app/services/families/locations.rb (1)
79-85: Enforce explicit timestamp ordering before serializing pointsMake ordering explicit at the last step so history is always chronologically stable.
Proposed fix
- points: sampled.pluck(:latitude, :longitude, :timestamp) + points: sampled.order(timestamp: :asc).pluck(:latitude, :longitude, :timestamp)🤖 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 79 - 85, The serialized points array is not guaranteed to be chronologically ordered; before building the points value for the hash (the points key using sampled), explicitly order the relation by timestamp (e.g., sampled.order(:timestamp)) so the pluck returns points in ascending time order and history is stable; update the code that sets points to use the ordered relation (or sort the plucked rows by the timestamp element) using the sampled relation to enforce deterministic chronological ordering.e2e/v2/realtime/family.spec.js (1)
148-159: Consider documenting the reliance on internal MapLibre API.Accessing
source._datais an internal MapLibre implementation detail that could break with library updates. While this may be the only practical way to inspect source contents in tests, consider:
- Adding a comment noting this is internal API
- Checking MapLibre release notes during upgrades
Add clarifying comment
const source = controller.map.getSource("family-source-history") + // Note: _data is internal MapLibre API - may break on library updates if (!source?._data?.features) return { hasSource: true, featureCount: 0, coordinateCounts: [] }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/v2/realtime/family.spec.js` around lines 148 - 159, The test relies on MapLibre internal state by accessing source._data via controller.map.getSource("family-source-history"); add an inline comment above that access explaining this is using MapLibre internal API (source._data) and may break on library upgrades, and mention that release notes should be checked when upgrading MapLibre; optionally wrap the access in a small helper function (e.g., getInternalSourceData) and add a sanity check (ensure source exists) so the test failure message makes the dependency clear.app/controllers/family/location_requests_controller.rb (1)
10-25: Consider using Turbo Streams for CRUD actions.Per coding guidelines, CRUD actions should respond with Turbo Streams instead of redirects. The
FlashStreamableconcern withstream_flash(:notice, "message")is recommended for flash messages.This can be deferred but would improve UX by avoiding full page reloads.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/controllers/family/location_requests_controller.rb` around lines 10 - 25, The create action in LocationRequestsController currently redirects after the Families::CreateLocationRequest call which triggers full page reloads; update create to respond to Turbo Stream by including the FlashStreamable concern and replacing redirect_to calls with stream_flash(:notice, ...) or stream_flash(:alert, ...) and render a turbo_stream response (successful and failure paths) so the UI updates without full reload; locate the create method and the Families::CreateLocationRequest invocation and change the response logic to call stream_flash and render turbo_stream (or head :ok with turbo_stream) instead of redirecting.app/models/concerns/user_family.rb (1)
127-134: Consider documenting the 1-year cap for 'all' history window.The comment on line 118 mentions "capped at 1 year maximum," but for the
'all'case on line 132, using1.year.agomay be surprising to users who expect "all" to mean their complete history. Consider either:
- Adding inline documentation explaining why this cap exists (performance/privacy)
- Making the cap configurable
This is acceptable for now but worth documenting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/concerns/user_family.rb` around lines 127 - 134, The 'all' branch in the window_start computation (using family_history_window and window_start) currently sets 1.year.ago as a cap without explanation; add a short inline comment above this case explaining the 1-year cap rationale (performance/privacy) and make the cap configurable by replacing the hardcoded 1.year with a constant or config value (e.g., USER_HISTORY_MAX_WINDOW or Settings.user_history.max_window) so callers can change it later; update the case to reference that constant and ensure any tests or usages reference the new config key.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a58c8111-a816-43fa-8e06-501442872bc6
⛔ Files ignored due to path filters (4)
Gemfile.lockis excluded by!**/*.lockapp/assets/images/GetItOnGooglePlay_Badge_Web_color_English.svgis excluded by!**/*.svgapp/assets/svg/icons/lucide/outline/file-up.svgis excluded by!**/*.svgapp/assets/svg/icons/lucide/outline/smartphone.svgis excluded by!**/*.svg
📒 Files selected for processing (99)
CHANGELOG.mdapp/assets/builds/tailwind.cssapp/assets/stylesheets/leaflet_theme.cssapp/controllers/api/v1/families/locations_controller.rbapp/controllers/api/v1/imports_controller.rbapp/controllers/families_controller.rbapp/controllers/family/location_requests_controller.rbapp/controllers/family/location_sharing_controller.rbapp/controllers/settings/background_jobs_controller.rbapp/controllers/settings/onboardings_controller.rbapp/controllers/users/registrations_controller.rbapp/controllers/visits_controller.rbapp/helpers/application_helper.rbapp/helpers/insights_helper.rbapp/helpers/stats_helper.rbapp/javascript/controllers/family_navbar_indicator_controller.jsapp/javascript/controllers/location_sharing_toggle_controller.jsapp/javascript/controllers/maps/maplibre_controller.jsapp/javascript/controllers/maps/maplibre_realtime_controller.jsapp/javascript/controllers/maps_controller.jsapp/javascript/controllers/onboarding_modal_controller.jsapp/javascript/maps/helpers.jsapp/javascript/maps_maplibre/layers/family_layer.jsapp/javascript/maps_maplibre/utils/route_segmenter.jsapp/jobs/families/expire_location_requests_job.rbapp/mailers/family_mailer.rbapp/models/concerns/calculateable.rbapp/models/concerns/user_family.rbapp/models/family/location_request.rbapp/models/family/membership.rbapp/services/countries_and_cities.rbapp/services/families/create_location_request.rbapp/services/families/locations.rbapp/services/families/update_location_sharing.rbapp/services/reverse_geocoding/places/fetch_data.rbapp/services/stats/calculate_month.rbapp/services/stats/hexagon_calculator.rbapp/services/users/safe_settings.rbapp/views/devise/registrations/_api_key.html.erbapp/views/devise/registrations/_points_usage.html.erbapp/views/devise/registrations/edit.html.erbapp/views/devise/registrations/new.html.erbapp/views/families/_location_sharing_toggle.html.erbapp/views/families/_navbar_indicator.html.erbapp/views/families/show.html.erbapp/views/family/location_requests/show.html.erbapp/views/family_mailer/location_request.html.erbapp/views/family_mailer/location_request.text.erbapp/views/home/index.html.erbapp/views/imports/index.html.erbapp/views/insights/_monthly_digest.html.erbapp/views/layouts/application.html.erbapp/views/map/_onboarding_modal.html.erbapp/views/map/maplibre/index.html.erbapp/views/places/index.html.erbapp/views/points/index.html.erbapp/views/settings/_navigation.html.erbapp/views/shared/_navbar.html.erbapp/views/shared/map/_date_navigation.html.erbapp/views/shared/map/_date_navigation_v2.html.erbapp/views/stats/index.html.erbapp/views/trips/_trip.html.erbapp/views/visits/index.html.erbconfig/initializers/03_dawarich_settings.rbconfig/initializers/filter_parameter_logging.rbconfig/routes.rbconfig/schedule.ymldb/migrate/20260313134546_create_family_location_requests.rbdb/migrate/20260314000001_fix_route_opacity_default.rbdb/schema.rbe2e/v2/map/layers/family.spec.jse2e/v2/map/layers/routes.spec.jse2e/v2/realtime/family.spec.jsspec/factories/family_location_requests.rbspec/helpers/application_helper_spec.rbspec/helpers/stats_helper_spec.rbspec/integration/family_privacy_spec.rbspec/jobs/families/expire_location_requests_job_spec.rbspec/mailers/family_mailer_spec.rbspec/models/concerns/user_family_spec.rbspec/models/family/location_request_spec.rbspec/models/trip_spec.rbspec/requests/api/v1/families/locations_spec.rbspec/requests/api/v1/imports_spec.rbspec/requests/family/location_requests_spec.rbspec/requests/settings/background_jobs_spec.rbspec/requests/settings/onboarding_spec.rbspec/requests/trips_spec.rbspec/requests/users/registrations_spec.rbspec/requests/visits_spec.rbspec/services/countries_and_cities_spec.rbspec/services/families/create_location_request_spec.rbspec/services/families/locations_spec.rbspec/services/reverse_geocoding/places/fetch_data_spec.rbspec/services/stats/hexagon_calculator_spec.rbspec/services/users/safe_settings_spec.rbspec/swagger/api/v1/imports_controller_spec.rbswagger/v1/swagger.yamlvendor/javascript/leaflet.control.layers.tree.js
Comment on lines
+399
to
405
| this.debouncedLoadFamilyHistory() | ||
| } | ||
|
|
||
| debouncedLoadFamilyHistory() { | ||
| if (this._familyHistoryTimer) clearTimeout(this._familyHistoryTimer) | ||
| this._familyHistoryTimer = setTimeout(() => this.loadFamilyHistory(), 300) | ||
| } |
There was a problem hiding this comment.
Cancel superseded family-history loads.
The debounce only delays calls; it does not cancel the previous fetch. If the user changes the window twice, an older response can arrive last and repaint the map/info with stale history. The timer also survives disconnect(), so this callback can run against a torn-down controller.
🛠️ One way to make the load lifecycle safe
disconnect() {
+ clearTimeout(this._familyHistoryTimer)
+ this._familyHistoryAbortController?.abort()
this._stopReplayPlayback()
this.settingsController?.stopRecalculationPolling()
this.searchManager?.destroy()
cancelAllPreviews()
this.cleanup.cleanup()
this.map?.remove()
performanceMonitor.logReport()
}
async loadFamilyHistory() {
+ this._familyHistoryAbortController?.abort()
+ const requestId = (this._familyHistoryRequestId || 0) + 1
+ this._familyHistoryRequestId = requestId
+ const abortController = new AbortController()
+ this._familyHistoryAbortController = abortController
+
try {
const startAt = this.startDateValue
const endAt = this.endDateValue
if (!startAt || !endAt) return
const params = new URLSearchParams({ start_at: startAt, end_at: endAt })
const response = await fetch(
`/api/v1/families/locations/history?${params}`,
{
+ signal: abortController.signal,
headers: {
Accept: "application/json",
"Content-Type": "application/json",
Authorization: `Bearer ${this.apiKeyValue}`,
},
},
)
+ if (requestId !== this._familyHistoryRequestId) return
if (!response.ok) return
const data = await response.json()
const members = data.members || []
...
} catch (error) {
+ if (error.name === "AbortError") return
console.error("[Maps V2] Failed to load family history:", error)
}
}As per coding guidelines, "Always clean up in disconnect()."
Also applies to: 1219-1261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/javascript/controllers/maps/maplibre_controller.js` around lines 399 -
405, debouncedLoadFamilyHistory currently only delays calls and leaves prior
timers/fetches alive; update it and disconnect() to fully cancel superseded
work: when debouncedLoadFamilyHistory runs, clear the existing
this._familyHistoryTimer as you do, and also abort any in-flight request by
calling this._familyHistoryAbort?.abort() and nulling it; in loadFamilyHistory
create a new AbortController, assign it to this._familyHistoryAbort and pass its
signal into the fetch/XHR so the request can be cancelled; finally, implement
cleanup in disconnect() to clearTimeout(this._familyHistoryTimer) and abort/null
this._familyHistoryAbort so timers and requests won't run against a torn-down
controller (also catch/ignore abort errors in loadFamilyHistory).
Comment on lines
+75
to
+87
| completeOnboarding() { | ||
| this.trackEvent("onboarding_completed") | ||
|
|
||
| if (this.onboardingUrlValue) { | ||
| fetch(this.onboardingUrlValue, { | ||
| method: "PATCH", | ||
| headers: { | ||
| "X-CSRF-Token": document.querySelector('meta[name="csrf-token"]') | ||
| ?.content, | ||
| "Content-Type": "application/json", | ||
| }, | ||
| }) | ||
| } |
There was a problem hiding this comment.
Don't mark onboarding complete with a fire-and-forget fetch().
This bypasses the Turbo/form submission path required in app/javascript and ignores failures or navigation aborts, so the modal can disappear locally while onboarding stays incomplete on the server. Submit a hidden form_with via requestSubmit() and let Turbo own the PATCH flow.
As per coding guidelines, "Do not use fetch() for form submissions—use form_with with Turbo. If custom headers are needed (e.g., API key), use Stimulus to submit the form via requestSubmit()."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/javascript/controllers/onboarding_modal_controller.js` around lines 75 -
87, The completeOnboarding() handler currently uses a fire-and-forget fetch to
PATCH onboardingUrlValue; instead create or target a hidden form (rendered via
Rails form_with) whose action equals onboardingUrlValue and method is PATCH, set
any needed headers/fields on that form, and call form.requestSubmit() so Turbo
handles submission and error/navigation flows; update completeOnboarding to
locate or create the hidden form element and invoke requestSubmit() rather than
using fetch(), referencing the completeOnboarding function and
onboardingUrlValue to locate the change.
Comment on lines
+144
to
+188
| appendToHistory(memberId, coords, color) { | ||
| const historySourceId = `${this.sourceId}-history` | ||
| const source = this.map.getSource(historySourceId) | ||
| if (!source) return | ||
|
|
||
| const data = source._data || { type: "FeatureCollection", features: [] } | ||
| const features = [...(data.features || [])] | ||
|
|
||
| const index = features.findIndex((f) => f.properties.userId === memberId) | ||
|
|
||
| if (index >= 0) { | ||
| // Append coordinate to existing polyline | ||
| features[index] = { | ||
| ...features[index], | ||
| geometry: { | ||
| type: "LineString", | ||
| coordinates: [...features[index].geometry.coordinates, coords], | ||
| }, | ||
| } | ||
| } else { | ||
| // No existing polyline — store the point so the next update creates a line | ||
| // A LineString needs at least 2 coordinates, so track pending starts | ||
| if (!this._pendingHistoryStarts) this._pendingHistoryStarts = {} | ||
|
|
||
| if (this._pendingHistoryStarts[memberId]) { | ||
| // We have a previous point, create the polyline | ||
| features.push({ | ||
| type: "Feature", | ||
| geometry: { | ||
| type: "LineString", | ||
| coordinates: [this._pendingHistoryStarts[memberId], coords], | ||
| }, | ||
| properties: { | ||
| userId: memberId, | ||
| color: color, | ||
| }, | ||
| }) | ||
| delete this._pendingHistoryStarts[memberId] | ||
| } else { | ||
| this._pendingHistoryStarts[memberId] = coords | ||
| return // Don't update source yet — need 2 points for a LineString | ||
| } | ||
| } | ||
|
|
||
| source.setData({ type: "FeatureCollection", features }) |
There was a problem hiding this comment.
Clean up the new history state when history is cleared or a member disappears.
appendToHistory() now keeps per-member pending coordinates, but clearHistory() never resets that cache, and removeMember() still only drops the point feature. After a clear/remove, the next update can connect a fresh point to a stale coordinate or leave an old trace visible for someone who stopped sharing.
🧹 Suggested follow-up
removeMember(memberId) {
const features = this.data?.features || []
const filtered = features.filter((f) => f.properties.id !== memberId)
this.update({
type: "FeatureCollection",
features: filtered,
})
+
+ if (this._pendingHistoryStarts) delete this._pendingHistoryStarts[memberId]
+
+ const historySource = this.map.getSource(`${this.sourceId}-history`)
+ const historyData = historySource?._data
+ if (historySource && historyData?.features) {
+ historySource.setData({
+ type: "FeatureCollection",
+ features: historyData.features.filter((f) => f.properties.userId !== memberId),
+ })
+ }
}
...
clearHistory() {
const historyLayerId = `${this.id}-history`
const historySourceId = `${this.sourceId}-history`
...
+ this._pendingHistoryStarts = {}
}Also applies to: 309-319
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/javascript/maps_maplibre/layers/family_layer.js` around lines 144 - 188,
appendToHistory introduced per-member pending starts in
this._pendingHistoryStarts but clearHistory() and removeMember() do not clear
that cache, causing stale coordinates to be reused; update clearHistory() to
reset this._pendingHistoryStarts = {} and update removeMember(memberId) to
delete this._pendingHistoryStarts[memberId] and also ensure it removes any
existing history LineString Feature for that user from the history source (use
the same historySourceId logic as appendToHistory to get the source and filter
out features where properties.userId === memberId), then write the cleaned
FeatureCollection back with source.setData.
Comment on lines
+6
to
+15
| belongs_to :requester, class_name: 'User' | ||
| belongs_to :target_user, class_name: 'User' | ||
| belongs_to :family | ||
|
|
||
| validates :requester_id, presence: true | ||
| validates :target_user_id, presence: true | ||
| validates :family_id, presence: true | ||
| validates :expires_at, presence: true | ||
| validate :requester_cannot_be_target | ||
|
|
There was a problem hiding this comment.
Validate that both users belong to the request family.
Right now any requester/target IDs are valid as long as family_id is present. One missed controller/service check is enough to create a cross-family request. This model should enforce that both users' memberships point at family.
🛡️ Example validation
validates :family_id, presence: true
validates :expires_at, presence: true
validate :requester_cannot_be_target
+ validate :members_belong_to_family
...
+ def members_belong_to_family
+ return unless family_id.present?
+
+ errors.add(:requester, 'must belong to the family') unless requester&.family_membership&.family_id == family_id
+ errors.add(:target_user, 'must belong to the family') unless target_user&.family_membership&.family_id == family_id
+ end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/family/location_request.rb` around lines 6 - 15, Add a model-level
validation to ensure both associated users belong to the same family as the
LocationRequest: add validate :users_belong_to_family to the LocationRequest
model and implement a users_belong_to_family method that loads requester and
target_user (safely handling nils) and compares their family_id to
self.family_id, adding errors.add(:requester_id, ...) and
errors.add(:target_user_id, ...) when mismatched; keep the existing
requester_cannot_be_target validation but use this new method to prevent
cross-family requests.
Comment on lines
+619
to
+683
| test.beforeAll(async ({ request }) => { | ||
| // Seed multiple historical points for family member 1 | ||
| // Points need distinct timestamps so they form a polyline (>= 2 points) | ||
| const now = Math.floor(Date.now() / 1000) | ||
|
|
||
| for (let i = 0; i < 5; i++) { | ||
| await sendOwnTracksPoint( | ||
| request, | ||
| API_KEYS.FAMILY_MEMBER_1, | ||
| TEST_LOCATIONS.BERLIN_CENTER.lat + i * 0.002, | ||
| TEST_LOCATIONS.BERLIN_CENTER.lon + i * 0.002, | ||
| now - (5 - i) * 3600, // Points spread over last 5 hours | ||
| ) | ||
| } | ||
| }) | ||
|
|
||
| test("family history API returns member history data", async ({ | ||
| request, | ||
| }) => { | ||
| // Call the history API directly | ||
| const today = new Date() | ||
| const startAt = new Date( | ||
| today.getFullYear(), | ||
| today.getMonth(), | ||
| today.getDate(), | ||
| 0, | ||
| 0, | ||
| 0, | ||
| ).toISOString() | ||
| const endAt = new Date( | ||
| today.getFullYear(), | ||
| today.getMonth(), | ||
| today.getDate(), | ||
| 23, | ||
| 59, | ||
| 59, | ||
| ).toISOString() | ||
|
|
||
| const response = await request.get( | ||
| `${process.env.BASE_URL || "http://localhost:3000"}/api/v1/families/locations/history?start_at=${startAt}&end_at=${endAt}`, | ||
| { | ||
| headers: { | ||
| Authorization: `Bearer ${API_KEYS.DEMO_USER}`, | ||
| "Content-Type": "application/json", | ||
| }, | ||
| }, | ||
| ) | ||
|
|
||
| // API should respond (may be 200 with data or 403 if not in family) | ||
| const status = response.status() | ||
| if (status === 200) { | ||
| const data = await response.json() | ||
| expect(data).toHaveProperty("members") | ||
| expect(Array.isArray(data.members)).toBe(true) | ||
|
|
||
| if (data.members.length > 0) { | ||
| const member = data.members[0] | ||
| expect(member).toHaveProperty("user_id") | ||
| expect(member).toHaveProperty("points") | ||
| expect(Array.isArray(member.points)).toBe(true) | ||
| } | ||
| } else { | ||
| // 403 = user not in family, which is expected in some test envs | ||
| expect([200, 403]).toContain(status) | ||
| } |
There was a problem hiding this comment.
Seed a fixed, authorized history window here.
This setup writes “last 5 hours” relative to Date.now(), but the API assertion only queries the current calendar day and then treats 403 as acceptable. Around midnight—or whenever the demo user is not in a family/history-sharing state—the test passes without exercising the history endpoint at all. Seed the family relationship + sharing settings in demo data, use timestamps inside the exact requested range, and require 200.
As per coding guidelines, "e2e/**/*.{js,ts}: Browser journeys should use seeded demo data to reset state".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/v2/map/layers/family.spec.js` around lines 619 - 683, The test currently
writes points relative to Date.now() and queries the current calendar day, which
can miss the seeded points; update the beforeAll to (1) seed a demo family
relationship and enable location sharing for API_KEYS.DEMO_USER and
API_KEYS.FAMILY_MEMBER_1 (ensure demo data reset), and (2) compute startAt/endAt
once in beforeAll (e.g., using the same calendar-day window used in the test)
and call sendOwnTracksPoint with timestamps that fall exactly inside that window
(use the same startAt/endAt-derived epoch seconds instead of now - ...), then
make the assertion require status 200 (remove accepting 403) so the history API
(members/points) is actually exercised; reference test.beforeAll,
sendOwnTracksPoint, API_KEYS.FAMILY_MEMBER_1, TEST_LOCATIONS.BERLIN_CENTER, and
API_KEYS.DEMO_USER to locate the changes.
Comment on lines
+701
to
+720
| // Wait for family members to load (triggers history load automatically) | ||
| const familyLoaded = await page | ||
| .waitForFunction( | ||
| () => { | ||
| const container = document.querySelector( | ||
| '[data-maps--maplibre-target="familyMembersContainer"]', | ||
| ) | ||
| if (!container) return false | ||
| return ( | ||
| container.querySelectorAll( | ||
| 'div[data-action*="centerOnFamilyMember"]', | ||
| ).length > 0 | ||
| ) | ||
| }, | ||
| { timeout: 15000 }, | ||
| ) | ||
| .then(() => true) | ||
| .catch(() => false) | ||
|
|
||
| if (!familyLoaded) return // Skip if no family members in this env |
There was a problem hiding this comment.
These map-history checks currently pass when nothing renders.
The familyLoaded guard returns early, and the missing-layer / empty-feature branches only log. That lets the suite go green even if the history layer is never created or populated. Once the demo state is deterministic, assert that the history source exists and that it contains at least one feature.
As per coding guidelines, "e2e/**/*.{js,ts}: Browser journeys should use seeded demo data to reset state".
Also applies to: 742-752, 815-828, 894-907
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/v2/map/layers/family.spec.js` around lines 701 - 720, The current
familyLoaded guard can silently pass when nothing renders; replace the
early-return check with explicit assertions: after the page.waitForFunction that
sets familyLoaded (using the container selector
'[data-maps--maplibre-target="familyMembersContainer"]' and data-action
'centerOnFamilyMember'), evaluate the map instance in the browser to assert the
history source (e.g., source id used by the history layer) exists and that its
GeoJSON/source features length is > 0; if the source is missing or has zero
features, fail the test (throw/assert) instead of logging/returning. Use the
same pattern for the other locations noted (around lines where similar waits
occur) so each branch verifies the history source and at least one feature is
present.
Comment on lines
+949
to
+973
| const memberInfos = await page.evaluate(() => { | ||
| const container = document.querySelector( | ||
| '[data-maps--maplibre-target="familyMembersContainer"]', | ||
| ) | ||
| if (!container) return [] | ||
|
|
||
| const infoElements = container.querySelectorAll("[data-member-info]") | ||
| return Array.from(infoElements).map((el) => ({ | ||
| userId: el.dataset.memberInfo, | ||
| text: el.textContent?.trim() || "", | ||
| visible: el.style.display !== "none", | ||
| })) | ||
| }) | ||
|
|
||
| if (memberInfos.length > 0) { | ||
| const visibleInfos = memberInfos.filter( | ||
| (info) => info.visible && info.text, | ||
| ) | ||
| if (visibleInfos.length > 0) { | ||
| // Info should contain sharing-related text (e.g., "Sharing since" or date) | ||
| console.log( | ||
| `[Test] Found ${visibleInfos.length} member info line(s): ${visibleInfos.map((i) => i.text).join(", ")}`, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
This test never verifies the rendered “sharing since” text.
After collecting the [data-member-info] rows, the success path only logs their contents. The test still passes if the label/date is wrong or missing entirely. Add an explicit expectation against the copy or date format for at least one visible member row.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/v2/map/layers/family.spec.js` around lines 949 - 973, The test currently
collects memberInfos via page.evaluate (querying
'[data-maps--maplibre-target="familyMembersContainer"]' and
'[data-member-info]') and only logs visibleInfos, so add an explicit assertion
that at least one visible member row contains the expected sharing text or date
format: after computing visibleInfos (variable visibleInfos), assert
visibleInfos.length > 0 and assert that visibleInfos.some(i => /Sharing
since|^\d{4}-\d{2}-\d{2}/.test(i.text)) (or a similar regex matching your app's
displayed date format) so the test fails if the sharing label/date is missing or
malformed.
Comment on lines
+6
to
+57
| describe '#normalize_country_name' do | ||
| let!(:country) do | ||
| Country.find_or_create_by!(name: 'Tanzania') do |c| | ||
| c.iso_a2 = 'TZ' | ||
| c.iso_a3 = 'TZA' | ||
| c.geom = 'MULTIPOLYGON (((0 0, 1 0, 1 1, 0 1, 0 0)))' | ||
| end | ||
| end | ||
|
|
||
| it 'returns canonical name for a known country variant' do | ||
| expect(helper.send(:normalize_country_name, 'Tanzania')).to eq('Tanzania') | ||
| end | ||
|
|
||
| it 'returns the original name when not found in Country table' do | ||
| expect(helper.send(:normalize_country_name, 'Unknown Land')).to eq('Unknown Land') | ||
| end | ||
|
|
||
| it 'returns nil for blank input' do | ||
| expect(helper.send(:normalize_country_name, nil)).to be_nil | ||
| expect(helper.send(:normalize_country_name, '')).to be_nil | ||
| end | ||
| end | ||
|
|
||
| describe '#collect_countries_and_cities (private)' do | ||
| let!(:tanzania) do | ||
| Country.find_or_create_by!(name: 'Tanzania') do |c| | ||
| c.iso_a2 = 'TZ' | ||
| c.iso_a3 = 'TZA' | ||
| c.geom = 'MULTIPOLYGON (((0 0, 1 0, 1 1, 0 1, 0 0)))' | ||
| end | ||
| end | ||
|
|
||
| let(:user) { create(:user) } | ||
|
|
||
| let(:stats) do | ||
| [ | ||
| create(:stat, user: user, year: 2025, month: 1, toponyms: [ | ||
| { 'country' => 'Tanzania', 'cities' => [{ 'city' => 'Dar es Salaam' }] } | ||
| ]), | ||
| create(:stat, user: user, year: 2025, month: 2, toponyms: [ | ||
| { 'country' => 'Tanzania', 'cities' => [{ 'city' => 'Arusha' }] } | ||
| ]) | ||
| ] | ||
| end | ||
|
|
||
| it 'deduplicates countries with canonical names' do | ||
| countries, cities = helper.send(:collect_countries_and_cities, stats) | ||
|
|
||
| expect(countries).to eq(['Tanzania']) | ||
| expect(cities).to contain_exactly('Dar es Salaam', 'Arusha') | ||
| end | ||
| end |
There was a problem hiding this comment.
Specs should validate public behavior, not private helpers via send.
Multiple expectations call private helper methods directly (helper.send(...)), which makes tests tightly coupled to implementation details.
As per coding guidelines: "spec/**/*.rb: 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/helpers/stats_helper_spec.rb` around lines 6 - 57, The tests are calling
private helpers via helper.send(:normalize_country_name) and
helper.send(:collect_countries_and_cities); replace these with specs that
exercise the public behavior that relies on those private methods instead.
Remove direct calls to normalize_country_name and collect_countries_and_cities;
instead invoke the public helper/view/controller method or rendered partial that
produces the country canonicalization and deduplicated city list (i.e., the
public entry point that uses those private methods), and assert the expected
output (canonical "Tanzania", deduplicated country list, and cities) so tests
verify observable behavior rather than implementation details.
Comment on lines
+129
to
+140
| it 'accepting a request enables sharing for the target user' do | ||
| result = Families::CreateLocationRequest.new(requester: user_b, target_user: user_a).call | ||
| expect(result.success?).to be true | ||
|
|
||
| request = result.payload[:request] | ||
|
|
||
| # Accept with 24h duration | ||
| user_a.update_family_location_sharing!(true, duration: '24h') | ||
| request.update!(status: :accepted, responded_at: Time.current) | ||
|
|
||
| expect(user_a.family_sharing_enabled?).to be true | ||
| expect(request.reload).to be_accepted |
There was a problem hiding this comment.
This example sidesteps the accept flow it claims to cover.
Line 136 enables sharing before the request is marked accepted, so the spec still passes if the real acceptance path stops flipping that flag. Drive the state change through the same accept action/service the app uses and assert on that side effect instead.
Based on learnings, "Test behavior, not implementation."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/integration/family_privacy_spec.rb` around lines 129 - 140, The test
directly flips the sharing flag with user_a.update_family_location_sharing! and
manually sets request.update!(status: :accepted...), which bypasses the real
accept flow; instead, remove those two calls and drive acceptance through the
app's acceptance API used elsewhere (e.g., call the request accept
service/action or model method the app uses to accept location requests, passing
user_a and duration: '24h'—for example replace the manual updates with the
equivalent of invoking the request acceptance service or
request.accept!(responder: user_a, duration: '24h')), then assert
user_a.family_sharing_enabled? and request.reload.be_accepted to verify the side
effects.
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (3)
app/javascript/controllers/onboarding_modal_controller.js (1)
75-90:⚠️ Potential issue | 🟠 MajorDon't mark onboarding complete with a fire-and-forget
fetch().This bypasses the Turbo/form submission path and silently ignores failures, so the modal can disappear locally while onboarding stays incomplete on the server.
As per coding guidelines: "Do not use
fetch()for form submissions—useform_withwith Turbo. If custom headers are needed, use Stimulus to submit the form viarequestSubmit()."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/controllers/onboarding_modal_controller.js` around lines 75 - 90, In completeOnboarding(), stop using a fire-and-forget fetch and instead submit the existing onboarding form via the browser/Turbo path (use requestSubmit() on the form element) so failures surface and Turbo handles the PATCH; locate the form in the controller (e.g., this.element.querySelector('form') or a new onboardingFormTarget) and call form.requestSubmit() (ensure the form has the correct action and method or a hidden _method='patch' input) rather than performing fetch(), removing the catch block.app/javascript/controllers/maps/maplibre_controller.js (1)
317-318:⚠️ Potential issue | 🟠 MajorCancel superseded family-history requests to avoid stale map/UI state.
Debouncing delays calls, but older in-flight requests can still resolve after newer ones and repaint stale history.
disconnect()currently clears only the timer, not active network work.🛠️ Suggested lifecycle-safe pattern
disconnect() { if (this._familyHistoryTimer) clearTimeout(this._familyHistoryTimer) + this._familyHistoryAbortController?.abort() + this._familyHistoryAbortController = null this._stopReplayPlayback() this.settingsController?.stopRecalculationPolling() this.searchManager?.destroy() cancelAllPreviews() this.cleanup.cleanup() this.map?.remove() performanceMonitor.logReport() } async loadFamilyHistory() { + this._familyHistoryAbortController?.abort() + const abortController = new AbortController() + this._familyHistoryAbortController = abortController + const requestId = (this._familyHistoryRequestId || 0) + 1 + this._familyHistoryRequestId = requestId + try { const startAt = this.startDateValue const endAt = this.endDateValue if (!startAt || !endAt) return const params = new URLSearchParams({ start_at: startAt, end_at: endAt }) const response = await fetch( `/api/v1/families/locations/history?${params}`, { + signal: abortController.signal, headers: { Accept: "application/json", "Content-Type": "application/json", Authorization: `Bearer ${this.apiKeyValue}`, }, }, ) + if (requestId !== this._familyHistoryRequestId) return if (!response.ok) return const data = await response.json() const members = data.members || [] ... } catch (error) { + if (error.name === "AbortError") return console.error("[Maps V2] Failed to load family history:", error) } }As per coding guidelines,
app/javascript/controllers/**/*_controller.js: "Always clean up indisconnect()."Also applies to: 403-406, 1220-1261
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/controllers/maps/maplibre_controller.js` around lines 317 - 318, disconnect() only clears the debounce timer and calls _stopReplayPlayback(), leaving in-flight family-history network requests to resolve and potentially repaint stale UI; introduce and use an AbortController (e.g., this._familyHistoryAbortController) for the family-history request code path (where family history fetches are started—locate the method that initiates the request, e.g., fetchFamilyHistory or wherever this._familyHistoryTimer triggers), pass its signal to fetch/axios, and in disconnect() call this._familyHistoryAbortController.abort() (and null it) in addition to clearing this._familyHistoryTimer and calling _stopReplayPlayback(); ensure any promise handlers check for abort/errors and skip updating map/UI when aborted.app/javascript/maps_maplibre/layers/family_layer.js (1)
145-190:⚠️ Potential issue | 🟠 MajorHistory cache cleanup is incomplete (stale lines/pending starts can leak across sessions).
clearHistory()removes map source/layer but leaves_historyFeaturesand_pendingHistoryStartsintact. Also, member removal should clear that member’s history state to prevent stale reconnects.🧹 Suggested cleanup patch
removeMember(memberId) { const features = this.data?.features || [] const filtered = features.filter((f) => f.properties.id !== memberId) this.update({ type: "FeatureCollection", features: filtered, }) + + if (this._pendingHistoryStarts) delete this._pendingHistoryStarts[memberId] + this._historyFeatures = (this._historyFeatures || []).filter( + (f) => f.properties.userId !== memberId, + ) + + const historySourceId = `${this.sourceId}-history` + const historySource = this.map.getSource(historySourceId) + if (historySource) { + historySource.setData({ + type: "FeatureCollection", + features: this._historyFeatures, + }) + } } clearHistory() { const historyLayerId = `${this.id}-history` const historySourceId = `${this.sourceId}-history` if (this.map.getLayer(historyLayerId)) { this.map.removeLayer(historyLayerId) } if (this.map.getSource(historySourceId)) { this.map.removeSource(historySourceId) } + this._historyFeatures = [] + this._pendingHistoryStarts = {} }Also applies to: 311-320
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/maps_maplibre/layers/family_layer.js` around lines 145 - 190, clearHistory currently removes the map source/layer but leaves in-memory caches, and member removal doesn't purge per-member history; update clearHistory (and any member-removal handler such as removeMember/handleMemberDisconnected) to fully clear this._historyFeatures = [] and this._pendingHistoryStarts = {} and ensure the history source is set to an empty FeatureCollection (use the same historySourceId / source.setData({ type: "FeatureCollection", features: [] }) pattern as in appendToHistory); additionally, when a member is removed, delete this._pendingHistoryStarts[memberId] and remove any feature where feature.properties.userId === memberId from this._historyFeatures and call source.setData(...) if the history source exists so stale lines/pending starts don't leak across sessions.
🧹 Nitpick comments (1)
app/javascript/controllers/onboarding_modal_controller.js (1)
61-73: Consider simplifying the dynamic target check.The current approach works, but the capitalization logic is verbose. Since the screen names are known and fixed, a simpler lookup or direct iteration would be clearer.
♻️ Optional simplification
switchScreen(targetName) { - const screens = ["choiceScreen", "importScreen", "trackScreen"] - for (const screen of screens) { - if ( - this[`has${screen.charAt(0).toUpperCase() + screen.slice(1)}Target`] - ) { - this[`${screen}Target`].classList.toggle( - "hidden", - screen !== targetName, - ) - } - } + const screenTargets = [ + { name: "choiceScreen", has: this.hasChoiceScreenTarget, el: () => this.choiceScreenTarget }, + { name: "importScreen", has: this.hasImportScreenTarget, el: () => this.importScreenTarget }, + { name: "trackScreen", has: this.hasTrackScreenTarget, el: () => this.trackScreenTarget }, + ] + for (const { name, has, el } of screenTargets) { + if (has) el().classList.toggle("hidden", name !== targetName) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/javascript/controllers/onboarding_modal_controller.js` around lines 61 - 73, The switchScreen method uses verbose capitalization to test dynamic "hasXTarget" properties; simplify by iterating the known screen names ("choiceScreen", "importScreen", "trackScreen") and checking the actual target property (e.g., this[`${screen}Target`]) or using an explicit mapping of names to target properties instead of building "has" property names — update switchScreen to loop screens, skip any undefined/null targets via this[`${screen}Target`], and call this[`${screen}Target`].classList.toggle("hidden", screen !== targetName) to show/hide screens.
🤖 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/family/location_requests_controller.rb`:
- Around line 63-67: The custom authorize_target_user! should be replaced with
Pundit authorization: add a before_action that loads the request (e.g.,
set_request) and calls authorize `@request` for the show, accept and decline
actions (e.g., authorize `@request`, :show? / :accept? / :decline? or :update?
depending on your policy), remove or stop using authorize_target_user!, and
implement corresponding policy methods (show?, accept?, decline?) in
LocationRequestPolicy so Pundit enforces access and raises
Pundit::NotAuthorizedError for unauthorized attempts.
- Around line 10-55: The create, accept, and decline actions in
Family::LocationRequestsController currently perform redirect_to with flash
messages; change them to return Turbo Stream responses using the FlashStreamable
pattern: replace redirects with stream_flash(...) calls and render Turbo Stream
responses (for success and failure) instead of redirecting; in create use the
result from Families::CreateLocationRequest to stream success or error, and in
accept/decline wrap state changes (current_user.update_family_location_sharing!
and `@request.update`!) in transactions as needed and then call stream_flash(...)
with the appropriate notice/alert and render the turbo_stream response so
clients receive updates without full redirects.
- Around line 31-53: The actionable? check is racy because state is read before
mutation; make the check and status change atomic by performing a conditional
update (or row lock) instead of relying on actionable? alone. For both accept
and decline, replace the pre-check with an atomic operation: inside a
transaction attempt to update the request only if it is still pending (e.g. use
Request.where(id: `@request.id`, status: :pending).update_all(status: :accepted,
responded_at: Time.current) or lock the row with `@request.lock`! and then
update), and only proceed to call current_user.update_family_location_sharing!
(in accept) or return success if the conditional update affected one row; if the
update did not succeed, treat it as non-actionable and redirect with the
existing alert. Use the method names accept, decline, actionable?,
`@request.update`!, and current_user.update_family_location_sharing! to locate and
modify the code.
In `@app/javascript/controllers/maps/maplibre_controller.js`:
- Around line 400-401: The call to this.debouncedLoadFamilyHistory is executed
even when family features are disabled; wrap the call in a guard that checks the
family feature flag before invoking the loader (e.g., if the controller exposes
a boolean like this.familyEnabled or a predicate method such as
this.isFamilyEnabled()). Update all places that call
this.debouncedLoadFamilyHistory (the month-change handler and the other
occurrence noted) to first check the feature flag and skip calling the method
when the flag is false to avoid unnecessary requests.
In `@app/javascript/controllers/onboarding_modal_controller.js`:
- Around line 37-39: The anonymous "close" listener added to modalTarget in
connect is never removed; store the handler as an instance property (e.g.,
this.onModalClose) and register it in connect
(this.modalTarget.addEventListener("close", this.onModalClose)) and remove it in
disconnect (this.modalTarget.removeEventListener("close", this.onModalClose"));
keep the handler body calling this.completeOnboarding() so the same function
reference can be removed, preventing duplicate handlers on reconnect.
In `@app/javascript/maps_maplibre/utils/route_segmenter.js`:
- Line 238: pointCount is overcounting because for multi-path routes the
synthetic IDL seam vertices in coordinates (an array of paths) are being
counted; change the multi-path branch to compute the sum of each subpath's
length and then subtract the number of seams (coordinates.length - 1) so only
original track points are counted (i.e., replace coordinates.flat().length with
a reduction over coordinates lengths minus (coordinates.length - 1)); keep the
single-path branch using segment.length and update the expression where
pointCount is assigned.
In `@app/jobs/data_migrations/fix_route_opacity_job.rb`:
- Around line 9-17: The current queries use (settings->>'route_opacity')::float
which will blow up on non-numeric values; modify both the count and the update
to first filter only rows where settings->>'route_opacity' is a numeric string
(e.g. add a PostgreSQL regex predicate like "(settings->>'route_opacity') ~
'^[0-9]*\\.?[0-9]+$'") and then apply the numeric cast and comparison (e.g. "...
AND (settings->>'route_opacity')::float > 1"), and apply the same
numeric-filtered WHERE to the User.where(...).update_all(Arel.sql(...)) call so
the jsonb_set operation only runs on rows guaranteed to cast to float.
In `@app/models/concerns/user_family.rb`:
- Around line 121-139: In family_history_points, guard against nil/invalid time
inputs before computing effective_start: validate that start_at and end_at are
present and are Time/DateTime-like (or coerce them safely), and return
Point.none if they are nil or not comparable; also ensure
family_sharing_started_at (used as started_at) is a valid time before using it
in the max() comparison. Add these checks at the top of family_history_points
(before computing window_start/effective_start) so the method returns Point.none
for bad inputs instead of raising when calling [start_at, started_at,
window_start].max or comparing effective_start >= end_at.
In `@db/migrate/20260314000001_fix_route_opacity_default.rb`:
- Around line 4-6: The migration's up method enqueues
DataMigrations::FixRouteOpacityJob with perform_later which runs asynchronously;
change DataMigrations::FixRouteOpacityJob.perform_later to
DataMigrations::FixRouteOpacityJob.perform_now inside the up method so the
migration blocks until the data fix completes (or alternatively implement the
needed SQL/data changes directly in this migration instead of calling the job),
and apply the same synchronous change for other migrations using perform_later
(e.g., 20260217000001_backfill_motion_data_from_raw_data.rb,
20251208210410_add_composite_index_to_stats.rb) if you want deterministic,
tied-to-migration execution.
---
Duplicate comments:
In `@app/javascript/controllers/maps/maplibre_controller.js`:
- Around line 317-318: disconnect() only clears the debounce timer and calls
_stopReplayPlayback(), leaving in-flight family-history network requests to
resolve and potentially repaint stale UI; introduce and use an AbortController
(e.g., this._familyHistoryAbortController) for the family-history request code
path (where family history fetches are started—locate the method that initiates
the request, e.g., fetchFamilyHistory or wherever this._familyHistoryTimer
triggers), pass its signal to fetch/axios, and in disconnect() call
this._familyHistoryAbortController.abort() (and null it) in addition to clearing
this._familyHistoryTimer and calling _stopReplayPlayback(); ensure any promise
handlers check for abort/errors and skip updating map/UI when aborted.
In `@app/javascript/controllers/onboarding_modal_controller.js`:
- Around line 75-90: In completeOnboarding(), stop using a fire-and-forget fetch
and instead submit the existing onboarding form via the browser/Turbo path (use
requestSubmit() on the form element) so failures surface and Turbo handles the
PATCH; locate the form in the controller (e.g.,
this.element.querySelector('form') or a new onboardingFormTarget) and call
form.requestSubmit() (ensure the form has the correct action and method or a
hidden _method='patch' input) rather than performing fetch(), removing the catch
block.
In `@app/javascript/maps_maplibre/layers/family_layer.js`:
- Around line 145-190: clearHistory currently removes the map source/layer but
leaves in-memory caches, and member removal doesn't purge per-member history;
update clearHistory (and any member-removal handler such as
removeMember/handleMemberDisconnected) to fully clear this._historyFeatures = []
and this._pendingHistoryStarts = {} and ensure the history source is set to an
empty FeatureCollection (use the same historySourceId / source.setData({ type:
"FeatureCollection", features: [] }) pattern as in appendToHistory);
additionally, when a member is removed, delete
this._pendingHistoryStarts[memberId] and remove any feature where
feature.properties.userId === memberId from this._historyFeatures and call
source.setData(...) if the history source exists so stale lines/pending starts
don't leak across sessions.
---
Nitpick comments:
In `@app/javascript/controllers/onboarding_modal_controller.js`:
- Around line 61-73: The switchScreen method uses verbose capitalization to test
dynamic "hasXTarget" properties; simplify by iterating the known screen names
("choiceScreen", "importScreen", "trackScreen") and checking the actual target
property (e.g., this[`${screen}Target`]) or using an explicit mapping of names
to target properties instead of building "has" property names — update
switchScreen to loop screens, skip any undefined/null targets via
this[`${screen}Target`], and call
this[`${screen}Target`].classList.toggle("hidden", screen !== targetName) to
show/hide screens.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 01591ad0-826e-40b4-8e4c-4c4bcd373267
📒 Files selected for processing (18)
app/controllers/family/location_requests_controller.rbapp/javascript/controllers/maps/maplibre_controller.jsapp/javascript/controllers/maps/maplibre_realtime_controller.jsapp/javascript/controllers/onboarding_modal_controller.jsapp/javascript/maps_maplibre/layers/family_layer.jsapp/javascript/maps_maplibre/utils/route_segmenter.jsapp/jobs/data_migrations/fix_route_opacity_job.rbapp/jobs/families/expire_location_requests_job.rbapp/models/concerns/user_family.rbapp/models/family/membership.rbapp/services/families/locations.rbdb/migrate/20260314000001_fix_route_opacity_default.rbspec/jobs/data_migrations/fix_route_opacity_job_spec.rbspec/jobs/families/expire_location_requests_job_spec.rbspec/models/concerns/user_family_spec.rbspec/requests/family/location_requests_spec.rbspec/requests/family/location_sharing_spec.rbspec/services/families/update_location_sharing_spec.rb
🚧 Files skipped from review as they are similar to previous changes (4)
- app/javascript/controllers/maps/maplibre_realtime_controller.js
- spec/models/concerns/user_family_spec.rb
- spec/requests/family/location_requests_spec.rb
- app/models/family/membership.rb
Comment on lines
+10
to
+55
| def create | ||
| target = current_user.family&.members&.find_by(id: params[:target_user_id]) | ||
|
|
||
| unless target | ||
| redirect_to family_path, alert: 'User not found in your family' | ||
| return | ||
| end | ||
|
|
||
| result = Families::CreateLocationRequest.new(requester: current_user, target_user: target).call | ||
|
|
||
| if result.success? | ||
| redirect_to family_path, notice: 'Location request sent successfully' | ||
| else | ||
| redirect_to family_path, alert: result.payload[:message] | ||
| end | ||
| end | ||
|
|
||
| def show | ||
| # View rendered by template | ||
| end | ||
|
|
||
| def accept | ||
| unless actionable? | ||
| redirect_to family_path, alert: 'This request has expired or already been responded to' | ||
| return | ||
| end | ||
|
|
||
| duration = params[:duration] || @request.suggested_duration | ||
| ActiveRecord::Base.transaction do | ||
| current_user.update_family_location_sharing!(true, duration: duration) | ||
| @request.update!(status: :accepted, responded_at: Time.current) | ||
| end | ||
|
|
||
| redirect_to family_path, notice: 'Location sharing enabled' | ||
| end | ||
|
|
||
| def decline | ||
| unless actionable? | ||
| redirect_to family_path, alert: 'This request has expired or already been responded to' | ||
| return | ||
| end | ||
|
|
||
| @request.update!(status: :declined, responded_at: Time.current) | ||
|
|
||
| redirect_to family_path, notice: 'Location request declined' | ||
| end |
There was a problem hiding this comment.
CRUD actions should respond via Turbo Streams, not redirects.
create, accept, and decline currently redirect_to with flashes instead of Turbo Stream responses via FlashStreamable.
As per coding guidelines app/controllers/**/*.rb: For CRUD actions (create, update, destroy), respond with Turbo Streams and use stream_flash(...).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/family/location_requests_controller.rb` around lines 10 - 55,
The create, accept, and decline actions in Family::LocationRequestsController
currently perform redirect_to with flash messages; change them to return Turbo
Stream responses using the FlashStreamable pattern: replace redirects with
stream_flash(...) calls and render Turbo Stream responses (for success and
failure) instead of redirecting; in create use the result from
Families::CreateLocationRequest to stream success or error, and in
accept/decline wrap state changes (current_user.update_family_location_sharing!
and `@request.update`!) in transactions as needed and then call stream_flash(...)
with the appropriate notice/alert and render the turbo_stream response so
clients receive updates without full redirects.
Comment on lines
+31
to
+53
| def accept | ||
| unless actionable? | ||
| redirect_to family_path, alert: 'This request has expired or already been responded to' | ||
| return | ||
| end | ||
|
|
||
| duration = params[:duration] || @request.suggested_duration | ||
| ActiveRecord::Base.transaction do | ||
| current_user.update_family_location_sharing!(true, duration: duration) | ||
| @request.update!(status: :accepted, responded_at: Time.current) | ||
| end | ||
|
|
||
| redirect_to family_path, notice: 'Location sharing enabled' | ||
| end | ||
|
|
||
| def decline | ||
| unless actionable? | ||
| redirect_to family_path, alert: 'This request has expired or already been responded to' | ||
| return | ||
| end | ||
|
|
||
| @request.update!(status: :declined, responded_at: Time.current) | ||
|
|
There was a problem hiding this comment.
Make actionable? check atomic with the status update.
Line 32/47 checks state before mutation, then Line 40/52 writes later. Concurrent accept/decline/expiry can pass the check and still write stale transitions.
🔧 Suggested direction
- unless actionable?
- redirect_to family_path, alert: 'This request has expired or already been responded to'
- return
- end
-
- duration = params[:duration] || `@request.suggested_duration`
- ActiveRecord::Base.transaction do
- current_user.update_family_location_sharing!(true, duration: duration)
- `@request.update`!(status: :accepted, responded_at: Time.current)
- end
+ duration = params[:duration] || `@request.suggested_duration`
+ updated = false
+ ActiveRecord::Base.transaction do
+ `@request.lock`!
+ break unless actionable?
+ current_user.update_family_location_sharing!(true, duration: duration)
+ updated = `@request.update`(status: :accepted, responded_at: Time.current)
+ raise ActiveRecord::Rollback unless updated
+ end
+
+ unless updated
+ redirect_to family_path, alert: 'This request has expired or already been responded to'
+ return
+ end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/family/location_requests_controller.rb` around lines 31 - 53,
The actionable? check is racy because state is read before mutation; make the
check and status change atomic by performing a conditional update (or row lock)
instead of relying on actionable? alone. For both accept and decline, replace
the pre-check with an atomic operation: inside a transaction attempt to update
the request only if it is still pending (e.g. use Request.where(id: `@request.id`,
status: :pending).update_all(status: :accepted, responded_at: Time.current) or
lock the row with `@request.lock`! and then update), and only proceed to call
current_user.update_family_location_sharing! (in accept) or return success if
the conditional update affected one row; if the update did not succeed, treat it
as non-actionable and redirect with the existing alert. Use the method names
accept, decline, actionable?, `@request.update`!, and
current_user.update_family_location_sharing! to locate and modify the code.
Comment on lines
+63
to
+67
| def authorize_target_user! | ||
| return if @request.target_user == current_user | ||
|
|
||
| redirect_to family_path, alert: 'You are not authorized to view this request' | ||
| end |
There was a problem hiding this comment.
Use Pundit policy authorization for request access.
authorize_target_user! is custom authorization logic, but this controller does not apply a Pundit policy check for show/accept/decline.
As per coding guidelines app/**/*.rb: Use Devise for authentication and Pundit for authorization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/controllers/family/location_requests_controller.rb` around lines 63 - 67,
The custom authorize_target_user! should be replaced with Pundit authorization:
add a before_action that loads the request (e.g., set_request) and calls
authorize `@request` for the show, accept and decline actions (e.g., authorize
`@request`, :show? / :accept? / :decline? or :update? depending on your policy),
remove or stop using authorize_target_user!, and implement corresponding policy
methods (show?, accept?, decline?) in LocationRequestPolicy so Pundit enforces
access and raises Pundit::NotAuthorizedError for unauthorized attempts.
Comment on lines
+400
to
+401
| this.debouncedLoadFamilyHistory() | ||
| } |
There was a problem hiding this comment.
Skip history loading when family is disabled.
This path runs on month changes even when family features are off, causing avoidable requests/work.
💡 Suggested guard
monthChanged(event) {
...
- this.debouncedLoadFamilyHistory()
+ if (this.settings?.familyEnabled) this.debouncedLoadFamilyHistory()
}
async loadFamilyHistory() {
+ if (!this.settings?.familyEnabled) return
try {
const startAt = this.startDateValue
const endAt = this.endDateValue
if (!startAt || !endAt) returnAlso applies to: 1220-1224
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/javascript/controllers/maps/maplibre_controller.js` around lines 400 -
401, The call to this.debouncedLoadFamilyHistory is executed even when family
features are disabled; wrap the call in a guard that checks the family feature
flag before invoking the loader (e.g., if the controller exposes a boolean like
this.familyEnabled or a predicate method such as this.isFamilyEnabled()). Update
all places that call this.debouncedLoadFamilyHistory (the month-change handler
and the other occurrence noted) to first check the feature flag and skip calling
the method when the flag is false to avoid unnecessary requests.
Comment on lines
37
to
39
| this.modalTarget.addEventListener("close", () => { | ||
| // Modal closed - state already saved | ||
| this.completeOnboarding() | ||
| }) |
There was a problem hiding this comment.
Clean up the modal "close" event listener in disconnect().
The anonymous listener added to modalTarget on line 37 is never removed. If the controller disconnects and reconnects (e.g., during Turbo navigation), this can cause duplicate handlers or stale references. Store the handler and remove it in disconnect().
🛡️ Proposed fix
export default class extends Controller {
static targets = ["modal", "choiceScreen", "importScreen", "trackScreen"]
static values = {
showable: Boolean,
onboardingUrl: String,
userTrial: Boolean,
importsCount: Number,
}
+
+ handleModalClose = () => {
+ this.completeOnboarding()
+ }
connect() {
if (this.showableValue) {
document.addEventListener("turbo:load", this.handleTurboLoad)
}
}
disconnect() {
document.removeEventListener("turbo:load", this.handleTurboLoad)
+ if (this.hasModalTarget) {
+ this.modalTarget.removeEventListener("close", this.handleModalClose)
+ }
}
// ...
checkAndShowModal() {
const MODAL_STORAGE_KEY = "dawarich_onboarding_shown"
const hasShownModal = localStorage.getItem(MODAL_STORAGE_KEY)
if (!hasShownModal && this.hasModalTarget) {
this.modalTarget.showModal()
localStorage.setItem(MODAL_STORAGE_KEY, "true")
this.trackEvent("onboarding_shown")
- this.modalTarget.addEventListener("close", () => {
- this.completeOnboarding()
- })
+ this.modalTarget.addEventListener("close", this.handleModalClose)
}
}Based on learnings: "Always clean up in disconnect() and prefer data-action attributes in HTML over addEventListener."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/javascript/controllers/onboarding_modal_controller.js` around lines 37 -
39, The anonymous "close" listener added to modalTarget in connect is never
removed; store the handler as an instance property (e.g., this.onModalClose) and
register it in connect (this.modalTarget.addEventListener("close",
this.onModalClose)) and remove it in disconnect
(this.modalTarget.removeEventListener("close", this.onModalClose")); keep the
handler body calling this.completeOnboarding() so the same function reference
can be removed, preventing duplicate handlers on reconnect.
| pointCount: segment.length, | ||
| startTime: startTime, | ||
| endTime: endTime, | ||
| pointCount: isMultiPath ? coordinates.flat().length : segment.length, |
There was a problem hiding this comment.
pointCount is inflated for IDL-crossing routes.
For multi-path routes, coordinates.flat().length counts synthetic seam vertices added at IDL boundaries, not just original track points.
💡 Suggested fix
- pointCount: isMultiPath ? coordinates.flat().length : segment.length,
+ pointCount: segment.length,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/javascript/maps_maplibre/utils/route_segmenter.js` at line 238,
pointCount is overcounting because for multi-path routes the synthetic IDL seam
vertices in coordinates (an array of paths) are being counted; change the
multi-path branch to compute the sum of each subpath's length and then subtract
the number of seams (coordinates.length - 1) so only original track points are
counted (i.e., replace coordinates.flat().length with a reduction over
coordinates lengths minus (coordinates.length - 1)); keep the single-path branch
using segment.length and update the expression where pointCount is assigned.
Comment on lines
+9
to
+17
| count = User.where("(settings->>'route_opacity')::float > 1").count | ||
| Rails.logger.info("Found #{count} users with route_opacity > 1") | ||
|
|
||
| return if count.zero? | ||
|
|
||
| User.where("(settings->>'route_opacity')::float > 1").update_all( | ||
| Arel.sql( | ||
| "settings = jsonb_set(settings, '{route_opacity}', to_jsonb((settings->>'route_opacity')::float / 100.0))" | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the full job file
cat -n app/jobs/data_migrations/fix_route_opacity_job.rbRepository: Freika/dawarich
Length of output: 877
🏁 Script executed:
# Check for similar JSON cast patterns in the codebase to see how they're handled
rg "settings.*::float" app/ --context 3Repository: Freika/dawarich
Length of output: 1214
🏁 Script executed:
# Also check for any migration patterns that handle malformed JSON/type conversions
rg "jsonb_set|update_all" app/jobs/ -A 5 -B 2Repository: Freika/dawarich
Length of output: 3958
🌐 Web query:
PostgreSQL update_all invalid float cast error handling behavior
💡 Result:
- In PostgreSQL, if an
UPDATEtries to store (or explicitlyCAST) a non-numeric string into afloat/double precision(e.g.,'','LOW_LOAD','1,100.00'), PostgreSQL raisesERROR: invalid input syntax for type double precisionwith SQLSTATE22P02(INVALID_TEXT_REPRESENTATION).[1][2] - That error fails the whole SQL statement (so you don’t get a “skip bad rows” behavior from a plain
UPDATE). If you are inside an explicit transaction (BEGIN ...), the transaction enters the “aborted” state and further commands are rejected until youROLLBACK.[3] - In Rails/ActiveRecord,
update_all:- skips validations/callbacks (it’s a direct SQL update).[4]
- still applies ActiveRecord’s normal type casting/serialization to values you pass in.[5]
- if PostgreSQL rejects the cast/update (e.g., invalid float text), the call raises an exception (typically
ActiveRecord::StatementInvalidwrapping a PG error), and the SQL update does not partially apply.[3]
Practical handling pattern is to prevent the invalid cast inside SQL (e.g., CASE WHEN ... THEN ... ELSE NULL END, or NULLIF(col,''), or a regex guard) so the statement never attempts to cast bad strings to double precision.[1][2]
Sources:
[1] https://www.postgrespro.com/list/thread-id/1232594
[2] https://stackoverflow.com/questions/62921487/postgresql-cant-convert-string-to-double-precision
[3] https://edu.postgrespro.com/postgresql_internals-14_parts1-4_en.pdf
[4] https://www.bigbinary.com/blog/rails-5-allows-updating-relation-objects-along-with-callbacks-and-validations
[5] https://apidock.com/rails/ActiveRecord/Relation/update_all
Guard the ::float cast to prevent job-wide failure on malformed JSON values.
On lines 9 and 14, (settings->>'route_opacity')::float will raise invalid input syntax for type double precision if any row contains non-numeric text (legacy data, manual edits, empty strings). This aborts the entire migration job with no partial updates.
Prevent the cast by filtering to only numeric values before attempting the conversion:
Recommended fix
def perform
Rails.logger.info('Starting route opacity fix job')
- count = User.where("(settings->>'route_opacity')::float > 1").count
+ scope = User
+ .where("settings ? 'route_opacity'")
+ .where("(settings->>'route_opacity') ~ '^[-+]?[0-9]*\\.?[0-9]+$'")
+ .where("(settings->>'route_opacity')::float > 1")
+
+ count = scope.count
Rails.logger.info("Found #{count} users with route_opacity > 1")
return if count.zero?
- User.where("(settings->>'route_opacity')::float > 1").update_all(
+ updated_count = scope.update_all(
Arel.sql(
"settings = jsonb_set(settings, '{route_opacity}', to_jsonb((settings->>'route_opacity')::float / 100.0))"
)
)
- Rails.logger.info("Completed route opacity fix job. Updated #{count} users")
+ Rails.logger.info("Completed route opacity fix job. Updated #{updated_count} users")
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/jobs/data_migrations/fix_route_opacity_job.rb` around lines 9 - 17, The
current queries use (settings->>'route_opacity')::float which will blow up on
non-numeric values; modify both the count and the update to first filter only
rows where settings->>'route_opacity' is a numeric string (e.g. add a PostgreSQL
regex predicate like "(settings->>'route_opacity') ~ '^[0-9]*\\.?[0-9]+$'") and
then apply the numeric cast and comparison (e.g. "... AND
(settings->>'route_opacity')::float > 1"), and apply the same numeric-filtered
WHERE to the User.where(...).update_all(Arel.sql(...)) call so the jsonb_set
operation only runs on rows guaranteed to cast to float.
Comment on lines
+121
to
+139
| def family_history_points(start_at:, end_at:) | ||
| return Point.none unless family_sharing_enabled? | ||
| return Point.none unless family_share_history? | ||
|
|
||
| started_at = family_sharing_started_at | ||
| return Point.none unless started_at | ||
|
|
||
| # Apply history window preference | ||
| window_start = case family_history_window | ||
| when '24h' then 24.hours.ago | ||
| when '7d' then 7.days.ago | ||
| when '30d' then 30.days.ago | ||
| when 'all' then 1.year.ago | ||
| else 24.hours.ago | ||
| end | ||
|
|
||
| effective_start = [start_at, started_at, window_start].max | ||
|
|
||
| return Point.none if effective_start >= end_at |
There was a problem hiding this comment.
Guard invalid/nil time inputs before calculating effective_start.
Line 137 assumes start_at and end_at are valid comparable time objects. If parsing upstream yields nil/invalid values, this can raise and return 500 instead of an empty relation.
🛡️ Proposed hardening
def family_history_points(start_at:, end_at:)
return Point.none unless family_sharing_enabled?
return Point.none unless family_share_history?
+ return Point.none unless start_at.respond_to?(:to_time) && end_at.respond_to?(:to_time)
+
+ start_at = start_at.to_time
+ end_at = end_at.to_time
started_at = family_sharing_started_at
return Point.none unless started_at
@@
effective_start = [start_at, started_at, window_start].max📝 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 family_history_points(start_at:, end_at:) | |
| return Point.none unless family_sharing_enabled? | |
| return Point.none unless family_share_history? | |
| started_at = family_sharing_started_at | |
| return Point.none unless started_at | |
| # Apply history window preference | |
| window_start = case family_history_window | |
| when '24h' then 24.hours.ago | |
| when '7d' then 7.days.ago | |
| when '30d' then 30.days.ago | |
| when 'all' then 1.year.ago | |
| else 24.hours.ago | |
| end | |
| effective_start = [start_at, started_at, window_start].max | |
| return Point.none if effective_start >= end_at | |
| def family_history_points(start_at:, end_at:) | |
| return Point.none unless family_sharing_enabled? | |
| return Point.none unless family_share_history? | |
| return Point.none unless start_at.respond_to?(:to_time) && end_at.respond_to?(:to_time) | |
| start_at = start_at.to_time | |
| end_at = end_at.to_time | |
| started_at = family_sharing_started_at | |
| return Point.none unless started_at | |
| # Apply history window preference | |
| window_start = case family_history_window | |
| when '24h' then 24.hours.ago | |
| when '7d' then 7.days.ago | |
| when '30d' then 30.days.ago | |
| when 'all' then 1.year.ago | |
| else 24.hours.ago | |
| end | |
| effective_start = [start_at, started_at, window_start].max | |
| return Point.none if effective_start >= end_at |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/concerns/user_family.rb` around lines 121 - 139, In
family_history_points, guard against nil/invalid time inputs before computing
effective_start: validate that start_at and end_at are present and are
Time/DateTime-like (or coerce them safely), and return Point.none if they are
nil or not comparable; also ensure family_sharing_started_at (used as
started_at) is a valid time before using it in the max() comparison. Add these
checks at the top of family_history_points (before computing
window_start/effective_start) so the method returns Point.none for bad inputs
instead of raising when calling [start_at, started_at, window_start].max or
comparing effective_start >= end_at.
Comment on lines
+4
to
+6
| def up | ||
| DataMigrations::FixRouteOpacityJob.perform_later | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine the migration file
find . -name "20260314000001_fix_route_opacity_default.rb" -type fRepository: Freika/dawarich
Length of output: 115
🏁 Script executed:
# Search for the migration file in db/migrate
fd "fix_route_opacity" db/migrate/Repository: Freika/dawarich
Length of output: 113
🏁 Script executed:
# Search for DataMigrations::FixRouteOpacityJob
rg "FixRouteOpacityJob" --type rb -B 2 -A 10Repository: Freika/dawarich
Length of output: 85
🏁 Script executed:
# Check for db/data directory to understand migration structure
find db -type d -name "data" 2>/dev/nullRepository: Freika/dawarich
Length of output: 66
🏁 Script executed:
# List migration files to understand pattern
ls -la db/migrate/ | head -20Repository: Freika/dawarich
Length of output: 1834
🏁 Script executed:
# Read the complete migration file
cat -n db/migrate/20260314000001_fix_route_opacity_default.rbRepository: Freika/dawarich
Length of output: 365
🏁 Script executed:
# Search for DataMigrations::FixRouteOpacityJob definition
rg "class FixRouteOpacityJob" -A 20Repository: Freika/dawarich
Length of output: 41
🏁 Script executed:
# Look for the job in app directory
find app -name "*fix_route_opacity*" -o -name "*opacity*" | grep -i jobRepository: Freika/dawarich
Length of output: 108
🏁 Script executed:
# Search more broadly for jobs directory
find app -type d -name "jobs"Repository: Freika/dawarich
Length of output: 85
🏁 Script executed:
# Read the job file
cat -n app/jobs/data_migrations/fix_route_opacity_job.rbRepository: Freika/dawarich
Length of output: 877
🏁 Script executed:
# Check if there are any other migrations using perform_later pattern
rg "perform_later" db/migrate/ -A 1 -B 1Repository: Freika/dawarich
Length of output: 2595
🏁 Script executed:
# Look for perform_now patterns in migrations
rg "perform_now" db/migrate/ -A 1 -B 1Repository: Freika/dawarich
Length of output: 41
Use synchronous execution in migrations instead of perform_later; async jobs decouple migration success from data completion.
perform_later enqueues the job asynchronously, meaning the migration succeeds (version recorded) before DataMigrations::FixRouteOpacityJob actually executes. If workers are down during deploy, data remains unfixed despite the schema migration being marked complete.
Replace perform_later with either:
perform_now(synchronous, blocks migration until completion)- Direct SQL/data manipulation in the migration itself
The coding guideline states: "Data manipulation migrations use the same ActiveRecord::Migration class and should run in the standard migration sequence"—implying deterministic execution tied to migration completion.
Note: This async pattern appears in multiple migrations (20260217000001_backfill_motion_data_from_raw_data.rb, 20251208210410_add_composite_index_to_stats.rb, etc.), suggesting a systematic architectural concern across data migrations.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@db/migrate/20260314000001_fix_route_opacity_default.rb` around lines 4 - 6,
The migration's up method enqueues DataMigrations::FixRouteOpacityJob with
perform_later which runs asynchronously; change
DataMigrations::FixRouteOpacityJob.perform_later to
DataMigrations::FixRouteOpacityJob.perform_now inside the up method so the
migration blocks until the data fix completes (or alternatively implement the
needed SQL/data changes directly in this migration instead of calling the job),
and apply the same synchronous change for other migrations using perform_later
(e.g., 20260217000001_backfill_motion_data_from_raw_data.rb,
20251208210410_add_composite_index_to_stats.rb) if you want deterministic,
tied-to-migration execution.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/services/families/locations.rb (1)
62-86: Consider usingsizeinstead ofcountto avoid an extra query when the relation is already loaded.Minor performance note:
points.count(line 65) always issues aCOUNT(*)query. If the relation might already be loaded or if you want to avoid the extra round-trip,points.sizewould use the cached length when available. However, given the potential size of point data, the current approach withcountis reasonable to avoid loading all records into memory just to check the count.🤖 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 62 - 86, Replace the use of points.count with points.size in build_family_history to let ActiveRecord use the relation's cached length when loaded (refer to the points local variable and the MAX_POINTS_PER_MEMBER sampling logic); change the conditional from total = points.count / next if total.zero? to total = points.size so you avoid an extra COUNT query when points is already loaded while keeping current behavior when it's not loaded.spec/jobs/data_migrations/backfill_onboarding_completed_job_spec.rb (1)
7-43: Add one spec for thesettings: nilpath.The job explicitly handles
COALESCE(settings, '{}'), but that branch isn’t asserted. Adding one legacy-row test would lock in that safeguard.✅ Suggested spec addition
describe '#perform' do + it 'handles users with nil settings' do + user = create(:user, points_count: 25, settings: nil) + + described_class.perform_now + + user.reload + expect(user.settings['onboarding_completed']).to be true + end + it 'sets onboarding_completed for users with points' do🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@spec/jobs/data_migrations/backfill_onboarding_completed_job_spec.rb` around lines 7 - 43, Add a spec that exercises the branch where a user's settings column is nil: create a user with settings: nil and a positive points_count (e.g., 10), call described_class.perform_now, reload the user, and assert that user.settings['onboarding_completed'] is true; use the same style as the existing examples (create(:user, points_count: ..., settings: nil) and described_class.perform_now) to ensure the COALESCE(settings, '{}') handling is covered.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 7-32: Changelog is missing or misclassifying new features from the
PR: update CHANGELOG.md by adding an "Added" section (above "Changed") that
lists the true new features: "Imports API (index/show/create) with UI and docs",
"family location-request workflow (model, controller, mailer, expiry job,
views)", and "family location history API and map-side history visualization";
move or reclassify the IDL-aware route segmentation (currently listed as a Fix)
into "Added" as "IDL-aware route segmentation" (reference unwrapCoordinates
behavior) and change "Fix family sharing not including the requesting user's own
location" to either a Fix or Added depending on whether family sharing was
introduced in this PR — if it was introduced here, move it to "Added" as "Family
sharing (initial implementation)"; ensure each added item references the unique
identifiers from the diff (Imports API (index/show/create), family
location-request workflow, family location history API, IDL-aware route
segmentation, unwrapCoordinates) and keep changelog phrasing consistent with
Keep a Changelog conventions.
---
Nitpick comments:
In `@app/services/families/locations.rb`:
- Around line 62-86: Replace the use of points.count with points.size in
build_family_history to let ActiveRecord use the relation's cached length when
loaded (refer to the points local variable and the MAX_POINTS_PER_MEMBER
sampling logic); change the conditional from total = points.count / next if
total.zero? to total = points.size so you avoid an extra COUNT query when points
is already loaded while keeping current behavior when it's not loaded.
In `@spec/jobs/data_migrations/backfill_onboarding_completed_job_spec.rb`:
- Around line 7-43: Add a spec that exercises the branch where a user's settings
column is nil: create a user with settings: nil and a positive points_count
(e.g., 10), call described_class.perform_now, reload the user, and assert that
user.settings['onboarding_completed'] is true; use the same style as the
existing examples (create(:user, points_count: ..., settings: nil) and
described_class.perform_now) to ensure the COALESCE(settings, '{}') handling is
covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 14abcf22-aeab-4370-ad74-3869eb93c3d5
📒 Files selected for processing (10)
.app_versionCHANGELOG.mdapp/javascript/controllers/maps/maplibre_realtime_controller.jsapp/jobs/data_migrations/backfill_onboarding_completed_job.rbapp/services/families/locations.rbdb/migrate/20260310000003_add_unique_index_to_place_visits.rbdb/migrate/20260315000001_backfill_onboarding_completed_for_existing_users.rbdb/schema.rbspec/jobs/data_migrations/backfill_onboarding_completed_job_spec.rbspec/requests/api/v1/families/locations_spec.rb
Comment on lines
+7
to
+32
| ## [1.3.4] - 2026-03-15 | ||
|
|
||
| ### Changed | ||
|
|
||
| - Redesigned onboarding modal with two paths: "I have data" (inline file import) and "Start tracking" (app download + QR code). New users with existing location data can now start importing within 2 clicks of signing up. | ||
| - Onboarding completion is now persisted server-side (`settings.onboarding_completed`) instead of relying solely on localStorage, preventing the modal from reappearing after browser data clears. | ||
| - Route opacity data migration now runs as a background job instead of inline during migration, improving deployment reliability for large instances. | ||
|
|
||
| ### Fixed | ||
|
|
||
| - Fix admin and supporter tooltip overflowing the page on narrow screens. #1449 | ||
| - Fix date navigation arrow tooltips overlapping with the navbar on map pages. #2229 #2100 | ||
| - Fix infinite loading spinner when a trip has no points in its date range. #2293 | ||
| - Fix Insights monthly digest panels disappearing when switching months. #2305 | ||
| - Fix suggested visit confirm/decline not removing the visit from the list. #2307 | ||
| - Fix Stats page reloading when clicking "countries, cities" link. #2270 | ||
| - Fix map base layer selection not being restored after page reload (Maps v1). #2093 | ||
| - Fix duplicate country names in stats caused by geocoder returning different spellings. #2044 | ||
| - Fix total distance display overlapping layer picker when distance is in miles. #2017 | ||
| - Fix default route opacity displaying as 6000% for new users. #1891 | ||
| - Fix shared month stats map missing hexagons from the last day of the month. #1934 | ||
| - Fix Nominatim reverse geocoder producing all places named "Suggested place" instead of actual place names. #2182 | ||
| - Fix IDL-crossing route segmenter returning inconsistent coordinate types. `unwrapCoordinates` now always returns a uniform array-of-arrays structure. #2038 | ||
| - Fix a migration taking too long. #2375 | ||
| - Fix family sharing not including the requesting user's own location. #2153 | ||
|
|
There was a problem hiding this comment.
Verify completeness of changelog entries.
The AI-generated summary indicates this PR "Adds server-persisted onboarding and a multi-screen onboarding modal, a family location-request workflow (model, controller, mailer, expiry job, views), a family location history API and map-side history visualization, a new Imports API (index/show/create) with UI and docs, IDL-aware route segmentation..."
However, the changelog only mentions:
- Family sharing as a fix (line 31) rather than a new feature
- IDL-crossing route segmenter as a fix (line 29) rather than a new feature
- No mention of new Imports API or family location-request workflow
If these are truly new features (models, controllers, API endpoints), they should appear in an "Added" section per Keep a Changelog conventions. If they're improvements to existing features, the current categorization may be correct.
Please verify that all significant changes from this PR are accurately reflected in the changelog.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CHANGELOG.md` around lines 7 - 32, Changelog is missing or misclassifying new
features from the PR: update CHANGELOG.md by adding an "Added" section (above
"Changed") that lists the true new features: "Imports API (index/show/create)
with UI and docs", "family location-request workflow (model, controller, mailer,
expiry job, views)", and "family location history API and map-side history
visualization"; move or reclassify the IDL-aware route segmentation (currently
listed as a Fix) into "Added" as "IDL-aware route segmentation" (reference
unwrapCoordinates behavior) and change "Fix family sharing not including the
requesting user's own location" to either a Fix or Added depending on whether
family sharing was introduced in this PR — if it was introduced here, move it to
"Added" as "Family sharing (initial implementation)"; ensure each added item
references the unique identifiers from the diff (Imports API
(index/show/create), family location-request workflow, family location history
API, IDL-aware route segmentation, unwrapCoordinates) and keep changelog
phrasing consistent with Keep a Changelog conventions.
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
5 participants
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
Chores / Data Migrations
Tests