Conversation
Retry point write contention across all ingestion paths
Tolerate null timestamps in the Null Island cleanup
Retry reverse geocoding point write timeouts
…rrors fix: quiet handled geocoder provider errors
Prevent reverse-geocoding place deadlocks
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReverse geocoding now classifies provider failures, retries broader database contention errors, and protects user-set place names. Visit scheduling, merging, and failure notifications are bounded more precisely. Legacy emails, migrations, null-island cleanup, and DNS resolution receive additional reliability handling. ChangesReverse-geocoding resilience
User-locked place names
Visit processing reliability
Stale email handling
Migration stability
Runtime support and release notes
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/services/reverse_geocoding/points/fetch_data.rb (1)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTransient-error warn logs lose useful detail for
Geocoder::ResponseParseError.All three sites log
e.messagefor TRANSIENT errors, butGeocoder::ResponseParseError#initialize(response)doesn't callsuper, so#messagereturns the class name rather than the parsed response — the one case where diagnostic content would matter most for a warn-level log.
app/services/reverse_geocoding/points/fetch_data.rb#L53-54: includee.class(and/ore.responsewhen present) alongsidee.messagein the warn log.app/services/places/name_fetcher.rb#L44-45: same adjustment to the warn log format.app/services/visits/names/fetcher.rb#L25-26: same adjustment to the warn log format.♻️ Proposed fix (example for fetch_data.rb)
rescue *ReverseGeocoding::ProviderErrors::TRANSIENT => e - Rails.logger.warn("Reverse geocoding provider error for point #{point.id}: #{e.message}") + Rails.logger.warn("Reverse geocoding provider error for point #{point.id}: #{e.class} - #{e.message}")Per geocoder's exceptions.rb,
ResponseParseError#initializeonly sets@responsewithout callingsuper.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/services/reverse_geocoding/points/fetch_data.rb` at line 1, Update the TRANSIENT warning logs in the fetch_data, name_fetcher, and visits names fetcher error handlers to include e.class and, when available, e.response alongside e.message. Preserve the existing warning context and handling while ensuring Geocoder::ResponseParseError logs its parsed response details.spec/services/reverse_geocoding/places/fetch_data_spec.rb (1)
439-448: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew spec tests a private method via
send.
save_placesis private; testing it directly viasendviolates the spec guideline to test through the public interface. Consider exercising this viaservice.callwith two geocoded results resolving to existing places, then asserting onPlace.upsert_all's argument ordering.As per coding guidelines, "Never test private methods via
send(). Test through the public interface instead."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/services/reverse_geocoding/places/fetch_data_spec.rb` around lines 439 - 448, Rewrite the ordering spec to exercise the public service.call interface instead of invoking private save_places via send. Configure two geocoded results that resolve to the existing first_place and second_place, then assert Place.upsert_all receives their attributes ordered by primary key with unique_by: :id.Source: Coding guidelines
spec/jobs/data_migrations/cleanup_null_island_job_spec.rb (1)
45-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify test execution and assertions.
The
not_to raise_errormatcher is generally discouraged in RSpec, as unexpected errors will inherently fail the test and provide a useful stack trace, whereasnot_to raise_errormasks that output. Executing the job inline yields a cleaner structure.♻️ Proposed refactor
it 'flags legacy points without timestamps and recalculates their tracks' do zero_point.update_column(:timestamp, nil) - expect { described_class.perform_now(user.id) }.not_to raise_error + described_class.perform_now(user.id) expect(Tracks::RecalculateJob).to have_been_enqueued.with(track.id) expect(Stats::CalculatingJob).not_to have_been_enqueued expect(zero_point.reload.anomaly).to be(true) end🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/jobs/data_migrations/cleanup_null_island_job_spec.rb` around lines 45 - 54, In the “flags legacy points without timestamps and recalculates their tracks” example, remove the expect block wrapping described_class.perform_now and invoke the job directly. Keep the existing enqueue and anomaly assertions unchanged so unexpected errors fail naturally with their stack traces.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/services/reverse_geocoding/points/fetch_data.rb`:
- Line 1: Update the TRANSIENT warning logs in the fetch_data, name_fetcher, and
visits names fetcher error handlers to include e.class and, when available,
e.response alongside e.message. Preserve the existing warning context and
handling while ensuring Geocoder::ResponseParseError logs its parsed response
details.
In `@spec/jobs/data_migrations/cleanup_null_island_job_spec.rb`:
- Around line 45-54: In the “flags legacy points without timestamps and
recalculates their tracks” example, remove the expect block wrapping
described_class.perform_now and invoke the job directly. Keep the existing
enqueue and anomaly assertions unchanged so unexpected errors fail naturally
with their stack traces.
In `@spec/services/reverse_geocoding/places/fetch_data_spec.rb`:
- Around line 439-448: Rewrite the ordering spec to exercise the public
service.call interface instead of invoking private save_places via send.
Configure two geocoded results that resolve to the existing first_place and
second_place, then assert Place.upsert_all receives their attributes ordered by
primary key with unique_by: :id.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1fb56b5b-3550-4735-b40e-4b445fa06378
📒 Files selected for processing (14)
CHANGELOG.mdapp/jobs/data_migrations/cleanup_null_island_job.rbapp/models/concerns/archivable.rbapp/services/places/name_fetcher.rbapp/services/reverse_geocoding/places/fetch_data.rbapp/services/reverse_geocoding/points/fetch_data.rbapp/services/reverse_geocoding/provider_errors.rbapp/services/visits/names/fetcher.rbspec/jobs/data_migrations/cleanup_null_island_job_spec.rbspec/models/concerns/archivable_spec.rbspec/services/places/name_fetcher_spec.rbspec/services/reverse_geocoding/places/fetch_data_spec.rbspec/services/reverse_geocoding/points/fetch_data_spec.rbspec/services/visits/names/fetcher_spec.rb
Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
Legacy Manager-owned trial lifecycle email types are skipped and logged instead of raising UnknownEmailType, so stale jobs stop retrying forever. Stale ActionMailer::MailDeliveryJob entries that bypass the wrapper are absorbed by no-op mailer actions, and mail addressed to a record that has since been hard-deleted is discarded rather than re-raising ActiveJob::DeserializationError.
Fix legacy trial email retries
Adds a regression example for non-String hosts that are not string-like (Integer), and covers the caching behaviour itself: a hostname resolves once and later calls are served from Rails.cache. Also records the fix in the changelog. (#3038)
Fix NoMethodError from the DNS cache when the resolver gets a nil name
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@spec/initializers/dns_cache_spec.rb`:
- Around line 24-29: Update the hostname used in the Resolv cache example around
getaddress_without_cache to be unique per test run, or explicitly clear/isolate
its Rails.cache entry before exercising the two Resolv.getaddress calls.
Preserve the expectation that both calls return the stubbed address and
getaddress_without_cache is invoked exactly once.
- Around line 17-18: Update the “returns them without a DNS lookup” example to
verify that IP literals bypass the cache by asserting neither Rails.cache read
nor write operations are invoked, while retaining the existing Resolv.getaddress
result assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 834de623-0f0a-4523-b464-9d3eaa7043a9
📒 Files selected for processing (3)
CHANGELOG.mdconfig/initializers/dns_cache.rbspec/initializers/dns_cache_spec.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
| it 'returns them without a DNS lookup' do | ||
| expect(Resolv.getaddress('127.0.0.1')).to eq('127.0.0.1') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that IP literals bypass the cache.
This example only verifies the returned value, so an implementation that still reads or writes Rails.cache would pass. Add expectations that both cache operations are not invoked.
As per coding guidelines, RSpec tests should test observable behavior rather than implementation details.
Suggested assertion
it 'returns them without a DNS lookup' do
+ expect(Rails.cache).not_to receive(:read)
+ expect(Rails.cache).not_to receive(:write)
expect(Resolv.getaddress('127.0.0.1')).to eq('127.0.0.1')
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it 'returns them without a DNS lookup' do | |
| expect(Resolv.getaddress('127.0.0.1')).to eq('127.0.0.1') | |
| it 'returns them without a DNS lookup' do | |
| expect(Rails.cache).not_to receive(:read) | |
| expect(Rails.cache).not_to receive(:write) | |
| expect(Resolv.getaddress('127.0.0.1')).to eq('127.0.0.1') |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/initializers/dns_cache_spec.rb` around lines 17 - 18, Update the
“returns them without a DNS lookup” example to verify that IP literals bypass
the cache by asserting neither Rails.cache read nor write operations are
invoked, while retaining the existing Resolv.getaddress result assertion.
Source: Coding guidelines
| allow(Resolv).to receive(:getaddress_without_cache).and_return('203.0.113.10') | ||
|
|
||
| expect(Resolv.getaddress('cache-me.invalid')).to eq('203.0.113.10') | ||
| expect(Resolv.getaddress('cache-me.invalid')).to eq('203.0.113.10') | ||
|
|
||
| expect(Resolv).to have_received(:getaddress_without_cache).once |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Isolate cache state in the hostname example.
The fixed hostname can already exist in Rails.cache from another example or test process. In that case, the first call returns the stale entry, the resolver stub is never exercised, and the example fails or gives misleading coverage. Use a unique hostname or isolate the cache boundary.
Suggested fix
it 'resolves once and serves later calls from the cache' do
+ hostname = "cache-me-#{SecureRandom.hex(8)}.invalid"
allow(Resolv).to receive(:getaddress_without_cache).and_return('203.0.113.10')
- expect(Resolv.getaddress('cache-me.invalid')).to eq('203.0.113.10')
- expect(Resolv.getaddress('cache-me.invalid')).to eq('203.0.113.10')
+ expect(Resolv.getaddress(hostname)).to eq('203.0.113.10')
+ expect(Resolv.getaddress(hostname)).to eq('203.0.113.10')📝 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.
| allow(Resolv).to receive(:getaddress_without_cache).and_return('203.0.113.10') | |
| expect(Resolv.getaddress('cache-me.invalid')).to eq('203.0.113.10') | |
| expect(Resolv.getaddress('cache-me.invalid')).to eq('203.0.113.10') | |
| expect(Resolv).to have_received(:getaddress_without_cache).once | |
| hostname = "cache-me-#{SecureRandom.hex(8)}.invalid" | |
| allow(Resolv).to receive(:getaddress_without_cache).and_return('203.0.113.10') | |
| expect(Resolv.getaddress(hostname)).to eq('203.0.113.10') | |
| expect(Resolv.getaddress(hostname)).to eq('203.0.113.10') | |
| expect(Resolv).to have_received(:getaddress_without_cache).once |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/initializers/dns_cache_spec.rb` around lines 24 - 29, Update the
hostname used in the Resolv cache example around getaddress_without_cache to be
unique per test run, or explicitly clear/isolate its Rails.cache entry before
exercising the two Resolv.getaddress calls. Preserve the expectation that both
calls return the stubbed address and getaddress_without_cache is invoked exactly
once.
Fixes #3204 by aligning the server-side render distance limit with Poster Studio.
- Support raster XYZ and style.json custom basemaps in Map v2 - Fall back to the default style when a custom basemap fails to load - fix: recover from unavailable custom basemaps
PostgreSQL raises QueryCanceled, not StatementTimeout, when statement_timeout fires, so the drop still aborted the migration on instances that set one. Rescue QueryAborted, which covers both. Wrap the job hand-off so an unreachable Redis logs and lets startup finish instead of re-raising into the crash loop this change removes. Flatten the job backoff to 5 minutes; polynomially_longer spanned days at the tail rather than the intended few hours.
A pooled connection can carry a session-level statement_timeout set by another job, which cancels the drop no matter how the lock race goes. Clear it for the duration of the drop and reset it afterwards. Capping retries re-raised into Sidekiq's own retry chain, turning a quiet wait into weeks of reported failures ending in the dead set. Narrow the enqueue rescue to connection failures so a NameError or a serialization bug surfaces instead of silently stranding the columns. Correct the comment claiming a waiting drop never queues ahead of writers: it does, and the short lock timeout only bounds the stall.
A bare SET and the following ALTER can land on different backends under PgBouncer transaction pooling, leaving the drop with no timeout and an unbounded ACCESS EXCLUSIVE request queued ahead of every points read and write. SET LOCAL inside an explicit transaction keeps both timeouts on the backend that runs the ALTER, matching Visits::StayPointDetector. Scoping the timeouts this way also removes both ensure blocks: a raise in RESET lock_timeout used to mask the LockWaitTimeout that retry_on needs to see, and a no-op run issued two RESETs for timeouts it never set. Cap the job's retries again and log once on exhaustion. Unlimited attempts stalled points writes every five minutes with no retry set, no dead set and no log line to find. Widen the enqueue rescue back to StandardError. A malformed REDIS_URL or an exhausted pool raises outside the connection-error families, and no enqueue failure is worth restarting the container for.
Five bugs in the visit-suggestion subsystem, plus review follow-ups. Visits::RealtimeDebouncer#clear was defined but never called, so the `nx: true` guard never released for a continuously-tracking user and realtime detection fired exactly once. VisitSuggestingJob now releases the key, guarded so a Redis blip can't drop the run under retry: false. The realtime lookback drops to 6h: clusters matching an existing visit never claim their points, so a 25h window re-detected and re-geocoded them on every run. Visits::TimeChunks discarded end_at whenever start and end shared a year, so the nightly job scanned from yesterday to 31 December. Three specs asserted that in their own titles; they were characterization tests, now rewritten. Visits::Suggest interpolated a backtrace into a user-facing notification and returned ExceptionReporter's value instead of an array. ExceptionReporter no-ops when self-hosted, so the backtrace now goes to Rails.logger unconditionally, and repeat notifications are gated by a Redis SET NX claim that fails open. Visits::Merger updated end_time and points but left duration, centre, radius and suggested_name at their pre-merge values, so Creator wrote the wrong duration and PlaceFinder resolved the first sub-cluster's place. The centre is recomputed on every absorption because can_merge_visits? compares against it; the rest is recomputed once when a chain closes, keeping the name if the geocoder lookup fails. Reverse geocoding overwrote user-chosen place names nightly, via FetchData#update_place, FetchData#populate_place_attributes (upsert_all, so callbacks cannot guard it) and Places::NameFetcher. A new places.name_locked_at, set on rename and on user-driven creation, protects them; renaming a place back to "Suggested place" hands it back to automatic naming. The lock state is exposed through both place serializers and surfaced in the Map v2 info panel and the drawer.
Poster Studio gains a 50-300% track width slider next to track opacity. The value is persisted as route_width and converted server-side into a 0.5-3.0 multiplier on the trackWidth style parameter, which until now was declared in style_builder.js but never set by any caller. Also covers the #3204 regression: a continent-wide frame completes instead of failing the area check.
Adds specs for a below-range width clamping to the 0.5 floor and a negative width falling back to 1.0, both previously unexercised, and records the new control in the changelog.
Every attempt queues an ACCESS EXCLUSIVE request that holds up each points reader and writer behind it. The lock is either free almost immediately or held by a long transaction a longer wait cannot outlast, so wait 1s instead of 5s and let boot try three times rather than ten — around 12s of startup instead of 185s, with the job as the real fallback. Neither dead end promises a rescue that will not come. The migration is recorded as applied whether or not the enqueue succeeds, and that enqueue is the only one in the codebase, so a failed hand-off and an exhausted job now both log the ALTER TABLE to run by hand.
Add a Poster Studio track width control
One second expires inside a batch. The same release backfills tracker ids and clears raw_data in 5k and 10k row batches that each hold RowExclusive on points for seconds, so the job would lose all 288 attempts against exactly the writers it has to wait out. At one try per five minutes a five second stall costs a fraction of a percent of the time, which the odds of ever finishing are worth. Boot keeps its short wait and three tries — a deploy should not be held up. Wrap both manual remedies in BEGIN and SET LOCAL so pasting one cannot queue the unbounded ACCESS EXCLUSIVE request this migration exists to avoid, and pin the tuned values so interpolating them into the SQL assertions cannot hide a bad edit.
Make visit suggestion consistent and predictable
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| def populate_place_attributes(place, data) | ||
| place.name = place_name(data) | ||
| place.name = place_name(data) unless place.name_locked? |
The area check built a flat-degree box around the poster centre while the renderer frames in Mercator. At the old 20 km cap the two agreed to within 0.00 degrees, but at continental distances they diverge: at 60N over 5,000 km the box sat 3.66 degrees north of the frame, so tracks inside the poster were rejected and tracks outside it were accepted and rendered off-frame. Latitude bounds now come from the same Mercator framing render.mjs uses, and longitude is compared with wrapping so a frame straddling the antimeridian no longer rejects a track three degrees from its centre. Poster Studio shares the geometry through poster_studio/render/frame_geometry and warns when a view is too wide for the largest poster area instead of silently zooming the saved poster in.
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
spec/services/reverse_geocoding/places/fetch_data_spec.rb (1)
483-493: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftAvoid testing private
save_placesand its payload shape directly.This test calls
service.send(:save_places, ...)and asserts an internalupsert_allarray, violating the spec guidelines to test observable behavior and avoid private-method testing. Exercise the behavior throughservice.calland verify persisted results or observable contention handling instead.As per coding guidelines, specs must test observable behavior and must never test private methods via
send().🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/services/reverse_geocoding/places/fetch_data_spec.rb` around lines 483 - 493, Replace the private save_places invocation in the “orders bulk updates by primary key” example with an observable service.call flow using the unsorted places as input. Verify the resulting persisted places or other public behavior that demonstrates deterministic primary-key ordering, without stubbing or asserting the internal upsert_all payload.Source: Coding guidelines
app/services/reverse_geocoding/places/fetch_data.rb (1)
148-160: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPersist
machine_namedin bulk writes.
update_placesetsplace.machine_named = true, but sibling name refreshes skip callbacks viaPlace.insert_all/Place.upsert_all, and the bulk payloads do not includemachine_named. Add the marker to both creation and update payloads so reverse-geocoded place names keep their naming metadata consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/services/reverse_geocoding/places/fetch_data.rb` around lines 148 - 160, Update the bulk payload construction in the reverse-geocoding persistence flow to include machine_named in both creation and update attribute hashes used by Place.insert_all/Place.upsert_all. Ensure the value reflects the marker set by update_place so refreshed place names retain consistent naming metadata.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/models/place.rb`:
- Around line 71-78: Update
ReverseGeocoding::Places::FetchData#populate_place_attributes and every other
machine-generated naming path to set machine_named before assigning or saving
the generated place name. Ensure lock_name_on_user_edit recognizes these changes
as machine-originated so they do not persist name_locked_at, while preserving
user-edit behavior for unmarked name changes.
In `@app/services/reverse_geocoding/places/fetch_data.rb`:
- Around line 39-50: Wrap the primary place update in update_place with the
existing bounded with_deadlock_retry helper, ensuring place.machine_named
assignment and place.update!(attributes) execute within the retry block.
Preserve the current attributes and update behavior while applying the same
contention policy used by Place#save_places and Place#upsert_all.
In `@app/services/visits/select_place.rb`:
- Line 18: Update the place-locking logic in the select-place flow to skip
name_locked_at updates when the place still has Place::DEFAULT_NAME. Preserve
locking for non-default names and rely on the model’s default-name handling so
Places::NameFetcher can replace the placeholder with a geocoded name.
In `@app/views/places/_drawer.html.erb`:
- Around line 17-19: Update the lock marker span in the place drawer to remove
the custom place-drawer__name-lock styling hook and use the project’s
Tailwind/DaisyUI utility classes for its styling, while preserving its title,
data-testid, lock icon, and naming behavior.
In `@CHANGELOG.md`:
- Line 16: Remove the duplicate Null Island changelog bullet near the top of
CHANGELOG.md, preserving the existing entry at line 22 with its timestamp-less
legacy-point detail.
In `@db/migrate/20260727120000_add_name_locked_at_to_places.rb`:
- Around line 4-7: Update the migration’s change method to remain reversible:
remove the column_exists? early return so Rails can automatically remove
name_locked_at on rollback, or replace change with explicit up and down methods
that add and remove the column respectively.
In `@spec/services/places/name_fetcher_spec.rb`:
- Around line 88-90: Update the example “still refreshes city and country” to
assert that service.call changes both place.city and place.country from their
initial values to the expected refreshed values, preserving the existing city
assertion while adding coverage for country updates.
In `@spec/services/reverse_geocoding/places/fetch_data_spec.rb`:
- Line 58: Resolve the Lint/AmbiguousBlockAssociation warnings in both
expectations around service.call by parenthesizing the matcher argument so the
block is unambiguously associated with expect. Apply the same adjustment to the
expectation at the second referenced location, then run bundle exec rubocop on
the modified Ruby file.
---
Outside diff comments:
In `@app/services/reverse_geocoding/places/fetch_data.rb`:
- Around line 148-160: Update the bulk payload construction in the
reverse-geocoding persistence flow to include machine_named in both creation and
update attribute hashes used by Place.insert_all/Place.upsert_all. Ensure the
value reflects the marker set by update_place so refreshed place names retain
consistent naming metadata.
In `@spec/services/reverse_geocoding/places/fetch_data_spec.rb`:
- Around line 483-493: Replace the private save_places invocation in the “orders
bulk updates by primary key” example with an observable service.call flow using
the unsorted places as input. Verify the resulting persisted places or other
public behavior that demonstrates deterministic primary-key ordering, without
stubbing or asserting the internal upsert_all payload.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 99ffed76-6031-4fd4-af04-1604003d32e4
📒 Files selected for processing (29)
CHANGELOG.mdapp/controllers/api/v1/places_controller.rbapp/controllers/places_controller.rbapp/javascript/controllers/maps/maplibre/data_loader.jsapp/javascript/controllers/maps/maplibre/event_handlers.jsapp/jobs/visit_suggesting_job.rbapp/models/place.rbapp/serializers/api/place_serializer.rbapp/services/places/name_fetcher.rbapp/services/reverse_geocoding/places/fetch_data.rbapp/services/visits/create.rbapp/services/visits/merger.rbapp/services/visits/realtime_debouncer.rbapp/services/visits/select_place.rbapp/services/visits/suggest.rbapp/services/visits/time_chunks.rbapp/views/places/_drawer.html.erbdb/migrate/20260727120000_add_name_locked_at_to_places.rbdb/schema.rbspec/jobs/visit_suggesting_job_spec.rbspec/models/place_spec.rbspec/requests/api/v1/places_spec.rbspec/requests/places_spec.rbspec/services/places/name_fetcher_spec.rbspec/services/reverse_geocoding/places/fetch_data_spec.rbspec/services/visits/merger_spec.rbspec/services/visits/select_place_spec.rbspec/services/visits/suggest_spec.rbspec/services/visits/time_chunks_spec.rb
| def lock_name_on_user_edit | ||
| return if machine_named | ||
| return unless will_save_change_to_name? | ||
|
|
||
| return self.name_locked_at = nil if name == DEFAULT_NAME | ||
| return if new_record? && !user_named | ||
|
|
||
| self.name_locked_at = Time.current |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Mark machine-originated name changes explicitly.
This callback treats every unmarked name change as a user edit. However, ReverseGeocoding::Places::FetchData#populate_place_attributes assigns place.name without setting machine_named, so its machine-generated name will be persisted with name_locked_at. Later geocoding will incorrectly preserve that stale name. Mark that path, and every other machine naming path, before saving.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/models/place.rb` around lines 71 - 78, Update
ReverseGeocoding::Places::FetchData#populate_place_attributes and every other
machine-generated naming path to set machine_named before assigning or saving
the generated place name. Ensure lock_name_on_user_edit recognizes these changes
as machine-originated so they do not persist name_locked_at, while preserving
user-edit behavior for unmarked name changes.
| attributes = { | ||
| lonlat: build_point_coordinates(data['geometry']['coordinates']), | ||
| city: data['properties']['city'], | ||
| country: data['properties']['country'], | ||
| geodata: data, | ||
| source: Place.sources[:photon], | ||
| reverse_geocoded_at: Time.current | ||
| ) | ||
| } | ||
| attributes[:name] = place_name(data) unless place.name_locked? | ||
|
|
||
| place.machine_named = true | ||
| place.update!(attributes) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file outline =="
ast-grep outline app/services/reverse_geocoding/places/fetch_data.rb || true
echo "== relevant source =="
sed -n '1,220p' app/services/reverse_geocoding/places/fetch_data.rb | cat -n
echo "== git diff stat =="
git diff --stat || true
echo "== searches for helper/usages =="
rg -n "with_deadlock_retry|Deadlocked|Deadlock|lock_wait|query_cancellation|upsert_all|machine_named" app/services/reverse_geocoding -S || trueRepository: Freika/dawarich
Length of output: 9138
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== reverse geocoding points helper =="
sed -n '1,120p' app/services/reverse_geocoding/points/fetch_data.rb | cat -n
echo "== model machine_named references =="
rg -n "machine_named|name_locked" app -S || true
echo "== ActiveRecord retry exceptions definitions usages =="
rg -n "ActiveRecord::ConnectionTimedOut|QueryCanceled|Deadlock|deadlock|query_cancellation|lock_wait_timeout" rails Gemfile . -S || trueRepository: Freika/dawarich
Length of output: 10493
🌐 Web query:
Rails 8.1.3 ActiveRecord::ConnectionTimedOut ActiveRecord::QueryCanceled ActiveRecord::Deadlocked retry_exceptions
💡 Result:
In Rails 8.1, ActiveRecord provides built-in mechanisms for handling retries, but they are focused on idempotent queries encountering specific connection errors rather than a general-purpose "retry_exceptions" configuration for all database exceptions [1][2][3]. Key distinctions for the exceptions mentioned: 1. ActiveRecord::ConnectionTimeoutError: This exception is raised when a connection cannot be obtained from the pool within the allowed timeout period [4]. It is not automatically retried by the adapter's connection retry mechanism because it indicates pool exhaustion rather than a transient connection failure [5][6]. 2. ActiveRecord::Deadlocked: This exception occurs when the database engine kills a transaction to resolve a deadlock [7]. Rails does not automatically retry deadlocked transactions at the adapter level because retrying them requires restarting the entire transaction block, which is not inherently safe to do automatically [7]. For deadlocks, the standard Rails pattern is to use ActiveJob's retry mechanism or manual rescue blocks [8]. For example: retry_on ActiveRecord::Deadlocked, wait: 5.seconds, attempts: 3 [8] 3. ActiveRecord::QueryCanceled: This generally relates to query timeouts or external cancellations. It is not part of the automatic connection-level retry logic [5][6]. Automatic Retry Logic Rails 8.1 automatically retries certain idempotent queries (such as simple association loads and #exists? checks) only when it encounters specific connection-related errors (e.g., lost connections) [1][2]. This behavior is governed by the database configuration in database.yml [3]: - connection_retries: Defaults to 1. Set to 0 to disable [6][3]. - retry_deadline: A duration (in seconds) after which even retryable queries will stop being retried [6][3]. There is no "retry_exceptions" configuration key in ActiveRecord; rather, the adapter internally defines which specific connection errors are considered retryable [5][6]. If you need to retry higher-level application errors or transactional issues like Deadlocked, you must implement that logic at the application level (e.g., using a retry library or ActiveJob) [8].
Citations:
- 1: https://github.com/rails/rails/blob/v8.1.2.1/activerecord/CHANGELOG.md
- 2: https://github.com/rails/rails/blob/8-1-stable/activerecord/CHANGELOG.md
- 3: rails/rails@6c70d02
- 4: https://api.rubyonrails.org/v8.1.1/classes/ActiveRecord/ConnectionTimeoutError.html
- 5: https://api.rubyonrails.org/v8.1.2.1/classes/ActiveRecord/ConnectionAdapters/AbstractAdapter.html
- 6: https://api.rubyonrails.org/v8.1/classes/ActiveRecord/ConnectionAdapters/AbstractAdapter.html
- 7: https://api.rubyonrails.org/v8.1.2.1/classes/ActiveRecord/Deadlocked.html
- 8: https://api.rubyonrails.org/v8.0/classes/ActiveJob/Exceptions/ClassMethods.html
Apply the bounded contention-retry policy to the primary place update.
update_place calls place.update!(attributes) without with_deadlock_retry, so Place#save_places and Place#upsert_all have built-in contention handling while contention on the original place stays immediate.
Proposed fix
- place.update!(attributes)
+ with_deadlock_retry { place.update!(attributes) }📝 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.
| attributes = { | |
| lonlat: build_point_coordinates(data['geometry']['coordinates']), | |
| city: data['properties']['city'], | |
| country: data['properties']['country'], | |
| geodata: data, | |
| source: Place.sources[:photon], | |
| reverse_geocoded_at: Time.current | |
| ) | |
| } | |
| attributes[:name] = place_name(data) unless place.name_locked? | |
| place.machine_named = true | |
| place.update!(attributes) | |
| attributes = { | |
| lonlat: build_point_coordinates(data['geometry']['coordinates']), | |
| city: data['properties']['city'], | |
| country: data['properties']['country'], | |
| geodata: data, | |
| source: Place.sources[:photon], | |
| reverse_geocoded_at: Time.current | |
| } | |
| attributes[:name] = place_name(data) unless place.name_locked? | |
| place.machine_named = true | |
| with_deadlock_retry { place.update!(attributes) } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/services/reverse_geocoding/places/fetch_data.rb` around lines 39 - 50,
Wrap the primary place update in update_place with the existing bounded
with_deadlock_retry helper, ensuring place.machine_named assignment and
place.update!(attributes) execute within the retry block. Preserve the current
attributes and update behavior while applying the same contention policy used by
Place#save_places and Place#upsert_all.
| def call | ||
| with_dedup_lock do | ||
| place = find_by_name_and_proximity || create_place | ||
| place.update!(name_locked_at: Time.current) unless place.name_locked? |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not lock the default placeholder name.
This direct update also locks Place::DEFAULT_NAME. That bypasses the model callback’s default-name rule, so Places::NameFetcher will preserve "Suggested place" instead of replacing it with a geocoded name.
Proposed fix
- place.update!(name_locked_at: Time.current) unless place.name_locked?
+ place.update!(name_locked_at: Time.current) unless place.name_locked? || place.name == Place::DEFAULT_NAME📝 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.
| place.update!(name_locked_at: Time.current) unless place.name_locked? | |
| place.update!(name_locked_at: Time.current) unless place.name_locked? || place.name == Place::DEFAULT_NAME |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/services/visits/select_place.rb` at line 18, Update the place-locking
logic in the select-place flow to skip name_locked_at updates when the place
still has Place::DEFAULT_NAME. Preserve locking for non-default names and rely
on the model’s default-name handling so Places::NameFetcher can replace the
placeholder with a geocoded name.
| <span class="place-drawer__name-lock" | ||
| title="You named this place, so automatic naming will not change it. Rename it to "<%= Place::DEFAULT_NAME %>" to hand it back to automatic naming." | ||
| data-testid="place-name-lock"><%= icon 'lock', class: 'w-4 h-4 inline' %></span> |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use Tailwind/DaisyUI classes for the new lock marker.
Replace the new place-drawer__name-lock styling hook with project-standard utility or DaisyUI classes.
As per coding guidelines, app/**/*.{css,scss,html,erb} must “Use Tailwind + DaisyUI for styling.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/views/places/_drawer.html.erb` around lines 17 - 19, Update the lock
marker span in the place drawer to remove the custom place-drawer__name-lock
styling hook and use the project’s Tailwind/DaisyUI utility classes for its
styling, while preserving its title, data-testid, lock icon, and naming
behavior.
Source: Coding guidelines
| def change | ||
| return if column_exists?(:places, :name_locked_at) | ||
|
|
||
| add_column :places, :name_locked_at, :datetime |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Rails files and migration:\n'
git ls-files | rg '(^config/application\.rb$|^Gemfile$|^Gemfile\.lock$|db/migrate/20260727120000_add_name_locked_at_to_places\.rb)$' || true
printf '\nMigration contents:\n'
cat -n db/migrate/20260727120000_add_name_locked_at_to_places.rb
printf '\nGemfile relevant Rails version:\n'
if [ -f Gemfile ]; then
rg -n "gem ['\"]rails['\"]|Rails \\(|VERSION|lock" Gemfile Gemfile.lock -A 3 -B 3 || true
fiRepository: Freika/dawarich
Length of output: 1963
🌐 Web query:
Rails Active Record migration change method reversible returns early does ActiveRecord handle irrevserible
💡 Result:
Active Record handles irreversible migrations in the change method by raising an ActiveRecord::IrreversibleMigration exception when you attempt to roll them back [1][2][3]. The change method is designed for migrations where Active Record can automatically determine how to reverse actions [4][1]. When you include an operation that the CommandRecorder cannot automatically invert (such as certain raw SQL executions or unsupported methods), Active Record will fail if that migration is reverted [5][6]. Regarding your question about "returning early" in the change method: 1. How Active Record processes change: Active Record tracks the commands within the change method using a CommandRecorder [5]. It does not simply execute them linearly and stop if it encounters an irreversible command; it records them for potential reversal [5][6]. 2. Irreversible operations: If you include an irreversible operation (e.g., executing arbitrary SQL that doesn't define a reverse path), the migration will still run successfully during an "up" (forward) migration [4][1]. However, if you subsequently run a "down" (rollback) command, the migration will raise an ActiveRecord::IrreversibleMigration error at runtime [1][2]. 3. Best Practices: If your migration contains irreversible operations, you should avoid using the change method entirely and instead define explicit up and down methods [4][2]. Alternatively, you can use the reversible block within the change method to explicitly define both the forward and backward logic [4][2]. If you find yourself needing to stop the migration process based on a condition, you can manually raise ActiveRecord::IrreversibleMigration at any point in your code to intentionally prevent the migration from being rolled back or to halt execution [2][7][8].
Citations:
- 1: https://api.rubyonrails.org/classes/ActiveRecord/Migration.html
- 2: https://api.rubyonrails.org/classes/ActiveRecord/IrreversibleMigration.html
- 3: https://api.rubyonrails.org/v8.0.0/classes/ActiveRecord/Migration.html
- 4: https://guides.rubyonrails.org/active_record_migrations.html
- 5: https://github.com/rails/rails/blob/main/activerecord/lib/active_record/migration/command_recorder.rb
- 6: https://api.rubyonrails.org/v7.2.3/classes/ActiveRecord/Migration/CommandRecorder.html
- 7: https://guides.rubyonrails.org/v7.2.2.1/active_record_migrations.html
- 8: https://github.com/rails/rails/blob/main/guides/source/active_record_migrations.md
🌐 Web query:
ActiveRecord::CommandRecorder records method calls on return early method body does not record execution
💡 Result:
The behavior described regarding ActiveRecord::Migration::CommandRecorder is a fundamental design feature of how it handles migration commands. The CommandRecorder does not "execute" the method calls in the traditional sense when they are first encountered; rather, it intercepts and stores them in an internal array (@commands) so they can be replayed or inverted later [1][2][3]. If you are experiencing a situation where a method appears not to be recorded—specifically when using an early return—it is important to understand that the CommandRecorder only captures calls that explicitly pass through the methods defined in the recorder [3][4]. Key points regarding this behavior: 1. Recording Mechanism: When a migration uses the change method, Rails executes the migration block through the CommandRecorder [1][5]. The recorder wraps database-modifying methods (like create_table or add_column) [3][4]. These methods are defined to call the record method internally [3][4]. 2. Early Returns: If you have custom logic or an early return statement in your migration, the methods that would normally trigger the recording process will be skipped if the code path is not executed [3]. The CommandRecorder cannot record calls that the Ruby interpreter never reaches. 3. Not an Execution Engine: The CommandRecorder is not a proxy that watches your entire migration execution; it is a delegated object that expects specific known methods to be invoked upon it [1][3]. If your migration code returns early before reaching these specific calls, the recorder has no knowledge of them [3]. If you find that a command is missing, ensure that the execution path of your migration code actually reaches the migration method call [3]. If you are implementing custom reversible behavior, you may need to use the reversible method or define explicit up and down methods to ensure the migration behaves as expected during both migration and rollback [3][6].
Citations:
- 1: https://api.rubyonrails.org/classes/ActiveRecord/Migration/CommandRecorder.html
- 2: https://api.rubyonrails.org/v8.0.5/classes/ActiveRecord/Migration/CommandRecorder.html
- 3: https://github.com/rails/rails/blob/94b5cd3a20edadd6f6b8cf0bdf1a4d4919df86cb/activerecord/lib/active_record/migration/command_recorder.rb
- 4: https://github.com/rails/rails/blob/d68e299167c8da07dc63a55197313b5c3396c3a4/activerecord/lib/active_record/migration/command_recorder.rb
- 5: https://api.rubyonrails.org/v8.1.3/classes/ActiveRecord/Migration/CommandRecorder.html
- 6: https://api.rubyonrails.org/v8.0.1/classes/ActiveRecord/Migration/CommandRecorder.html
Keep this migration reversible.
On rollback, column_exists? is true and change returns before invoking remove_column, so the migration finishes successfully while leaving the already-existing column in place. Remove the guard, or use explicit up/down methods that mirror each other.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@db/migrate/20260727120000_add_name_locked_at_to_places.rb` around lines 4 -
7, Update the migration’s change method to remain reversible: remove the
column_exists? early return so Rails can automatically remove name_locked_at on
rollback, or replace change with explicit up and down methods that add and
remove the column respectively.
| it 'still refreshes city and country' do | ||
| expect { service.call }.to change(place, :city).from(nil).to('New York') | ||
| end |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the country refresh promised by this example.
The example says it refreshes both city and country, but only checks city. A regression to country updates would pass unnoticed.
Proposed fix
- expect { service.call }.to change(place, :city).from(nil).to('New York')
+ service.call
+ expect(place.reload).to have_attributes(city: 'New York', country: 'United States')📝 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.
| it 'still refreshes city and country' do | |
| expect { service.call }.to change(place, :city).from(nil).to('New York') | |
| end | |
| it 'still refreshes city and country' do | |
| service.call | |
| expect(place.reload).to have_attributes(city: 'New York', country: 'United States') | |
| end |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/services/places/name_fetcher_spec.rb` around lines 88 - 90, Update the
example “still refreshes city and country” to assert that service.call changes
both place.city and place.country from their initial values to the expected
refreshed values, preserving the existing city assertion while adding coverage
for country updates.
| let(:place) { create(:place, name: "Mum's house", name_locked_at: 1.day.ago) } | ||
|
|
||
| it 'keeps the user-supplied name' do | ||
| expect { service.call }.not_to change { place.reload.name } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the ambiguous RSpec block association.
RuboCop reports Lint/AmbiguousBlockAssociation for both expectations. Parenthesize the matcher argument:
Proposed fix
- expect { service.call }.not_to change { place.reload.name }
+ expect { service.call }.not_to(change { place.reload.name })
...
- expect { service.call }.not_to change { sibling.reload.name }
+ expect { service.call }.not_to(change { sibling.reload.name })As per coding guidelines, always run RuboCop on modified Ruby files before committing: bundle exec rubocop <files>.
Also applies to: 90-90
🧰 Tools
🪛 GitHub Check: rubocop
[failure] 58-58:
Lint/AmbiguousBlockAssociation: Parenthesize the param change { place.reload.name } to make sure that the block will be associated with the change method call.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/services/reverse_geocoding/places/fetch_data_spec.rb` at line 58,
Resolve the Lint/AmbiguousBlockAssociation warnings in both expectations around
service.call by parenthesizing the matcher argument so the block is
unambiguously associated with expect. Apply the same adjustment to the
expectation at the second referenced location, then run bundle exec rubocop on
the modified Ruby file.
Sources: Coding guidelines, Linters/SAST tools
Stop the legacy lat/lon column drop from crash-looping startup
fix: judge poster saves against the rendered frame (+ track width control)
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@spec/migrations/drop_legacy_lat_lon_from_points_spec.rb`:
- Around line 7-28: Refactor the migration specs to exercise the public `up`
entry point instead of calling `migration.send(:drop_legacy_columns)`. Remove
direct stubs on the subject for `execute`, `column_exists?`, and `sleep`; stub
the underlying database connection or other external collaborators instead,
while preserving the existing retry assertions through `up` and the
`stub_drop_raising` helper.
- Around line 37-54: Replace the perform_later stubs and have_received
assertions in the “hands the drop to a background job once attempts are
exhausted” and “retries until the lock is acquired instead of failing on the
first loss” examples with
have_enqueued_job(DataMigrations::DropLegacyLatLonJob), matching the sibling
test’s enqueue assertion pattern. Preserve the existing expectations that the
job is enqueued only after retries are exhausted and not enqueued when the lock
succeeds.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6e499580-aadd-44cd-a32d-60e14ff98cd2
📒 Files selected for processing (5)
CHANGELOG.mdapp/jobs/data_migrations/drop_legacy_lat_lon_job.rbdb/migrate/20260714090000_drop_legacy_lat_lon_from_points.rbspec/jobs/data_migrations/drop_legacy_lat_lon_job_spec.rbspec/migrations/drop_legacy_lat_lon_from_points_spec.rb
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
| it 'hands the drop to a background job once attempts are exhausted' do | ||
| stub_drop_raising(ActiveRecord::LockWaitTimeout) | ||
| allow(DataMigrations::DropLegacyLatLonJob).to receive(:perform_later) | ||
|
|
||
| migration.send(:drop_legacy_columns) | ||
|
|
||
| expect(DataMigrations::DropLegacyLatLonJob).to have_received(:perform_later) | ||
| end | ||
|
|
||
| it 'retries until the lock is acquired instead of failing on the first loss' do | ||
| attempts = stub_drop_raising(ActiveRecord::LockWaitTimeout, times: 2) | ||
| allow(DataMigrations::DropLegacyLatLonJob).to receive(:perform_later) | ||
|
|
||
| migration.send(:drop_legacy_columns) | ||
|
|
||
| expect(attempts.call).to eq(3) | ||
| expect(DataMigrations::DropLegacyLatLonJob).not_to have_received(:perform_later) | ||
| end |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Prefer have_enqueued_job over mocking perform_later for asserting enqueue behavior.
These two tests stub DataMigrations::DropLegacyLatLonJob.perform_later and assert on have_received, while the sibling test at Lines 72-76 correctly uses have_enqueued_job(DataMigrations::DropLegacyLatLonJob). The same pattern should be used here for consistency and to avoid mocking an internal collaborator's class method.
As per coding guidelines, "Prefer have_enqueued_job over expect(Job).to receive(:perform_later) for testing background job enqueueing."
♻️ Proposed fix
it 'hands the drop to a background job once attempts are exhausted' do
stub_drop_raising(ActiveRecord::LockWaitTimeout)
- allow(DataMigrations::DropLegacyLatLonJob).to receive(:perform_later)
-
- migration.send(:drop_legacy_columns)
-
- expect(DataMigrations::DropLegacyLatLonJob).to have_received(:perform_later)
+ expect { migration.send(:drop_legacy_columns) }.to have_enqueued_job(DataMigrations::DropLegacyLatLonJob)
end
it 'retries until the lock is acquired instead of failing on the first loss' do
attempts = stub_drop_raising(ActiveRecord::LockWaitTimeout, times: 2)
- allow(DataMigrations::DropLegacyLatLonJob).to receive(:perform_later)
-
- migration.send(:drop_legacy_columns)
+ expect { migration.send(:drop_legacy_columns) }.not_to have_enqueued_job(DataMigrations::DropLegacyLatLonJob)
expect(attempts.call).to eq(3)
- expect(DataMigrations::DropLegacyLatLonJob).not_to have_received(:perform_later)
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| it 'hands the drop to a background job once attempts are exhausted' do | |
| stub_drop_raising(ActiveRecord::LockWaitTimeout) | |
| allow(DataMigrations::DropLegacyLatLonJob).to receive(:perform_later) | |
| migration.send(:drop_legacy_columns) | |
| expect(DataMigrations::DropLegacyLatLonJob).to have_received(:perform_later) | |
| end | |
| it 'retries until the lock is acquired instead of failing on the first loss' do | |
| attempts = stub_drop_raising(ActiveRecord::LockWaitTimeout, times: 2) | |
| allow(DataMigrations::DropLegacyLatLonJob).to receive(:perform_later) | |
| migration.send(:drop_legacy_columns) | |
| expect(attempts.call).to eq(3) | |
| expect(DataMigrations::DropLegacyLatLonJob).not_to have_received(:perform_later) | |
| end | |
| it 'hands the drop to a background job once attempts are exhausted' do | |
| stub_drop_raising(ActiveRecord::LockWaitTimeout) | |
| expect { migration.send(:drop_legacy_columns) }.to have_enqueued_job(DataMigrations::DropLegacyLatLonJob) | |
| end | |
| it 'retries until the lock is acquired instead of failing on the first loss' do | |
| attempts = stub_drop_raising(ActiveRecord::LockWaitTimeout, times: 2) | |
| expect { migration.send(:drop_legacy_columns) }.not_to have_enqueued_job(DataMigrations::DropLegacyLatLonJob) | |
| expect(attempts.call).to eq(3) | |
| end |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/migrations/drop_legacy_lat_lon_from_points_spec.rb` around lines 37 -
54, Replace the perform_later stubs and have_received assertions in the “hands
the drop to a background job once attempts are exhausted” and “retries until the
lock is acquired instead of failing on the first loss” examples with
have_enqueued_job(DataMigrations::DropLegacyLatLonJob), matching the sibling
test’s enqueue assertion pattern. Preserve the existing expectations that the
job is enqueued only after retries are exhausted and not enqueued when the lock
succeeds.
Source: Coding guidelines
MapLibre only fires style.load when it builds a Style from scratch; its default setStyle path diffs the document into the live style silently. Every style swap therefore stripped the app's data layers without ever re-adding them, and the custom-style error listener stayed armed forever, so the first failed tile request discarded a perfectly good basemap and reverted to the default style. - Pass diff: false on every setStyle call, via a shared swapStyle helper - Only treat a failure of the style document itself as a style failure, not the tile, sprite and glyph requests it spawns - Align classifyBasemapUrl with the API's style_json_url? so the browser no longer accepts URLs the API rejects - Disable tile category and POI toggles under a raster or foreign-style basemap, where they have nothing to act on
Add custom raster and style basemap URLs
A one-time backfill sets name_locked_at for places whose name a user set before 1.10.2, so the next reverse geocoding run no longer overwrites pre-upgrade renames. Machine-generated names are recognised by recomputing both historical formats from stored geodata and stay unlocked; places without geodata are locked conservatively. Also dedupes the (0,0) changelog bullet.
3 participants
Summary by CodeRabbit